[PATCH] removed unneccesary assigning to rc triggering unused value error in coverity

[Gedare Bloom]

On Thu, Feb 27, 2020 at 9:30 AM Joel Sherrill <joel at rtems.org> wrote:
>
>
>
> On Thu, Feb 27, 2020 at 9:48 AM Gedare Bloom <gedare at rtems.org> wrote:
>>
>> Hi Suyash,
>>
>> I have a  few comments for you.
>>
>> First, the commit message should follow the guidance at
>> https://docs.rtems.org/branches/master/eng/vc-users.html#creating-a-patch
>> and follow through the link about Commit Messages. We like to have a
>> short "tag" at the start of the commit to indicate the area of RTEMS
>> the commit applies to, in this case, maybe "bsps/shared: ignore return
>> value in display driver"  -- We don't currently have a set of standard
>> tags.
>
>
> The tag before the colon should be as specific as possible. This only
> impacts one file not a lot under bsps/shared. I do something like this:
>
> bsps/shared/.../disp_hcms29xx.c   Address unused return code warning
>
> Calls to rtems methods were putting the return status into a local variable
> and not testing it. This patch adds debug assert to check the value and
> annotation that it is deliberately ignored when not in debug.
>
> Coverity ID...
>
The CID is optional in commit message, but Joel likes to see it ;)

>>
>> Second, avoid making unnecessary changes in whitespace elsewhere in
>> source code. My guess is that your text editor did this for you
>> automatically. You may want to investigate how to disable it from
>> rewriting the entire file for formatting on open.
>
>
> Yes please. If the white space changes are needed because the file
> doesn't follow the RTEMS style, make a separate patch.
>
>>
>>
>> Third, if a return value is unused, there are two options to consider:
>> (a) it should be used, or (b) it should be ignored. Either way, we
>> would prefer to be explicit about the programmer intent. In this case,
>> does it make sense to check the return status of
>> rtems_semaphore_release? Can it fail? Would it matter if it does?
>>
>> To explicitly ignore a return value, you can do this instead:
>>
>> rc = rtems_semaphore_release(...)
>> (void) rc;
>>
>> As directed by our coding standards: "Use ‘(void) unused;’ to mark
>> unused parameters and set-but-unused variables immediately after being
>> set."
>
>
> RTEMS is old and some of the code pre-dates having a debug build. I would
> lean to something like this these days (as indicated in my comment above):
>
>   rc = rtems_semaphore_release(...)
>   RTEMS_DEBUG_ASSERT(rc == RTEMS_SUCCESSFUL);
>   (void) rc;
>
> It will be unused unless Debug is defined. And that might not be the
> right macro name.
>
> Similarly, I wouldn't mind seeing calls which do (void) rtems_...
> also do a debug assert. But that should be broadly discussed
> in the community. It is just a bad habit to ignore return values.
>
> It's funny how a VERY VERY simple change for an obvious warning
> can lead to what feel like out of proportion discussions. LOL
>
Yeah, there's a lot to unpack here. it almost might be worth having a macro..
#ifdef RTEMS_DEBUG
RTEMS_DEBUG_ASSERT_RV(err, cond) RTEMS_DEBUG_ASSERT(err == cond)
#else
RTEMS_DEBUG_ASSERT_RV(err, cond) (void) err;
#endif

Thinking out loud here.

> --joel
>
>>
>>
>>
>> On Thu, Feb 27, 2020 at 3:24 AM suyash singh <suyashsingh234 at gmail.com> wrote:
>> >
>> > ---
>> >  bsps/shared/dev/display/disp_hcms29xx.c | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/bsps/shared/dev/display/disp_hcms29xx.c b/bsps/shared/dev/display/disp_hcms29xx.c
>> > index 5730b36ea9..9d3e7220cf 100644
>> > --- a/bsps/shared/dev/display/disp_hcms29xx.c
>> > +++ b/bsps/shared/dev/display/disp_hcms29xx.c
>> > @@ -530,7 +530,7 @@ static rtems_task disp_hcms29xx_update_task
>> >  +---------------------------------------------------------------------------+
>> >  | Input Parameters:                                                         |
>> >  \*-------------------------------------------------------------------------*/
>> > -   rtems_task_argument argument
>> > +   rtems_task_argument argument
>> >  )
>> >  /*-------------------------------------------------------------------------*\
>> >  | Return Value:                                                             |
>> > @@ -597,7 +597,7 @@ static rtems_task disp_hcms29xx_update_task
>> >           (int) strlen(softc_ptr->disp_param.disp_buffer);
>> >        }
>> >        if (rc == RTEMS_SUCCESSFUL) {
>> > -       rc = rtems_semaphore_release(softc_ptr->disp_param.trns_sema_id);
>> > +        rtems_semaphore_release(softc_ptr->disp_param.trns_sema_id);
>> >        }
>> >        /*
>> >         * set initial offset to negative value
>> > @@ -911,7 +911,7 @@ static rtems_driver_address_table disp_hcms29xx_ops = {
>> >
>> >  static disp_hcms29xx_drv_t disp_hcms29xx_drv_tbl = {
>> >    {/* public fields */
>> > -    .ops =         &disp_hcms29xx_ops,
>> > +    .ops =         &disp_hcms29xx_ops,
>> >      .size =        sizeof (disp_hcms29xx_drv_t),
>> >    },
>> >    { /* our private fields */
>> > @@ -927,6 +927,6 @@ static disp_hcms29xx_drv_t disp_hcms29xx_drv_tbl = {
>> >    }
>> >  };
>> >
>> > -rtems_libi2c_drv_t *disp_hcms29xx_driver_descriptor =
>> > +rtems_libi2c_drv_t *disp_hcms29xx_driver_descriptor =
>> >    &disp_hcms29xx_drv_tbl.libi2c_drv_entry;
>> >
>> > --
>> > 2.17.1
>> >
>> > _______________________________________________
>> > devel mailing list
>> > devel at rtems.org
>> > http://lists.rtems.org/mailman/listinfo/devel
>> _______________________________________________
>> devel mailing list
>> devel at rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel