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

[Joel Sherrill]

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...


> 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

--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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20200227/61a4c0df/attachment-0001.html>