On Thu, Feb 27, 2020 at 9:48 AM Gedare Bloom <ged...@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 <suyashsingh...@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@rtems.org > > http://lists.rtems.org/mailman/listinfo/devel > _______________________________________________ > devel mailing list > devel@rtems.org > http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
