On Thu, Feb 27, 2020 at 9:30 AM Joel Sherrill <j...@rtems.org> wrote:
>
>
>
> 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...
>
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 <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
