<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 27, 2020 at 9:48 AM Gedare Bloom <<a href="mailto:gedare@rtems.org">gedare@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Suyash,<br>
<br>
I have a few comments for you.<br>
<br>
First, the commit message should follow the guidance at<br>
<a href="https://docs.rtems.org/branches/master/eng/vc-users.html#creating-a-patch" rel="noreferrer" target="_blank">https://docs.rtems.org/branches/master/eng/vc-users.html#creating-a-patch</a><br>
and follow through the link about Commit Messages. We like to have a<br>
short "tag" at the start of the commit to indicate the area of RTEMS<br>
the commit applies to, in this case, maybe "bsps/shared: ignore return<br>
value in display driver" -- We don't currently have a set of standard<br>
tags.<br></blockquote><div><br></div><div>The tag before the colon should be as specific as possible. This only</div><div>impacts one file not a lot under bsps/shared. I do something like this:</div><div><br></div><div><span style="color:rgb(80,0,80)">bsps/shared/.../disp</span><span style="color:rgb(80,0,80)">_hcms29xx.c</span> Address unused return code warning<br></div><div><br></div><div>Calls to rtems methods were putting the return status into a local variable</div><div>and not testing it. This patch adds debug assert to check the value and</div><div>annotation that it is deliberately ignored when not in debug.</div><div><br></div><div>Coverity ID...</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Second, avoid making unnecessary changes in whitespace elsewhere in<br>
source code. My guess is that your text editor did this for you<br>
automatically. You may want to investigate how to disable it from<br>
rewriting the entire file for formatting on open.<br></blockquote><div><br></div><div>Yes please. If the white space changes are needed because the file</div><div>doesn't follow the RTEMS style, make a separate patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Third, if a return value is unused, there are two options to consider:<br>
(a) it should be used, or (b) it should be ignored. Either way, we<br>
would prefer to be explicit about the programmer intent. In this case,<br>
does it make sense to check the return status of<br>
rtems_semaphore_release? Can it fail? Would it matter if it does?<br>
<br>
To explicitly ignore a return value, you can do this instead:<br>
<br>
rc = rtems_semaphore_release(...)<br>
(void) rc;<br>
<br>
As directed by our coding standards: "Use ‘(void) unused;’ to mark<br>
unused parameters and set-but-unused variables immediately after being<br>
set."<br></blockquote><div><br></div><div>RTEMS is old and some of the code pre-dates having a debug build. I would</div><div>lean to something like this these days (as indicated in my comment above):</div><div><br></div><div> rc = rtems_semaphore_release(...)<br> RTEMS_DEBUG_ASSERT(rc == RTEMS_SUCCESSFUL);</div><div> (void) rc; <br></div><div><br></div><div>It will be unused unless Debug is defined. And that might not be the</div><div>right macro name.</div><div><br></div><div>Similarly, I wouldn't mind seeing calls which do (void) rtems_...</div><div>also do a debug assert. But that should be broadly discussed</div><div>in the community. It is just a bad habit to ignore return values.</div><div><br></div><div>It's funny how a VERY VERY simple change for an obvious warning</div><div>can lead to what feel like out of proportion discussions. LOL</div><div><br></div><div>--joel</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
On Thu, Feb 27, 2020 at 3:24 AM suyash singh <<a href="mailto:suyashsingh234@gmail.com" target="_blank">suyashsingh234@gmail.com</a>> wrote:<br>
><br>
> ---<br>
> bsps/shared/dev/display/disp_hcms29xx.c | 8 ++++----<br>
> 1 file changed, 4 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/bsps/shared/dev/display/disp_hcms29xx.c b/bsps/shared/dev/display/disp_hcms29xx.c<br>
> index 5730b36ea9..9d3e7220cf 100644<br>
> --- a/bsps/shared/dev/display/disp_hcms29xx.c<br>
> +++ b/bsps/shared/dev/display/disp_hcms29xx.c<br>
> @@ -530,7 +530,7 @@ static rtems_task disp_hcms29xx_update_task<br>
> +---------------------------------------------------------------------------+<br>
> | Input Parameters: |<br>
> \*-------------------------------------------------------------------------*/<br>
> - rtems_task_argument argument<br>
> + rtems_task_argument argument<br>
> )<br>
> /*-------------------------------------------------------------------------*\<br>
> | Return Value: |<br>
> @@ -597,7 +597,7 @@ static rtems_task disp_hcms29xx_update_task<br>
> (int) strlen(softc_ptr->disp_param.disp_buffer);<br>
> }<br>
> if (rc == RTEMS_SUCCESSFUL) {<br>
> - rc = rtems_semaphore_release(softc_ptr->disp_param.trns_sema_id);<br>
> + rtems_semaphore_release(softc_ptr->disp_param.trns_sema_id);<br>
> }<br>
> /*<br>
> * set initial offset to negative value<br>
> @@ -911,7 +911,7 @@ static rtems_driver_address_table disp_hcms29xx_ops = {<br>
><br>
> static disp_hcms29xx_drv_t disp_hcms29xx_drv_tbl = {<br>
> {/* public fields */<br>
> - .ops = &disp_hcms29xx_ops,<br>
> + .ops = &disp_hcms29xx_ops,<br>
> .size = sizeof (disp_hcms29xx_drv_t),<br>
> },<br>
> { /* our private fields */<br>
> @@ -927,6 +927,6 @@ static disp_hcms29xx_drv_t disp_hcms29xx_drv_tbl = {<br>
> }<br>
> };<br>
><br>
> -rtems_libi2c_drv_t *disp_hcms29xx_driver_descriptor =<br>
> +rtems_libi2c_drv_t *disp_hcms29xx_driver_descriptor =<br>
> &disp_hcms29xx_drv_tbl.libi2c_drv_entry;<br>
><br>
> --<br>
> 2.17.1<br>
><br>
> _______________________________________________<br>
> devel mailing list<br>
> <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
_______________________________________________<br>
devel mailing list<br>
<a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
<a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a></blockquote></div></div>