diff mbox

[3/4] dtrace-probe: Put semicolon after while on its own line

Message ID 1498076108-29914-4-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi June 21, 2017, 8:15 p.m. UTC 
  clang shows this warning.

  /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: error: while loop has empty body [-Werror,-Wempty-body]
            while (*p++ != '\0' && p - strtab < strtab_size);
                                                            ^
  /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: note: put the semicolon on a separate line to silence this warning

Putting the semicolon on its own line is not a big sacrifice to get rid of this
warning.  I think it's also useful to keep this, because it can catch errors
like this:

  while (something);
    {
      ...
    }

although gcc would warn about it in a different way (misleading indentation).

This warning is already discussed here in the GCC bugzilla:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62184

gdb/ChangeLog:

	* dtrace-probe.c (dtrace_process_dof_probe): Put semi-colon on
	its own line.
---
 gdb/dtrace-probe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Sergio Durigan Junior June 21, 2017, 8:34 p.m. UTC | #1 
On Wednesday, June 21 2017, Simon Marchi wrote:

> clang shows this warning.
>
>   /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: error: while loop has empty body [-Werror,-Wempty-body]
>             while (*p++ != '\0' && p - strtab < strtab_size);
>                                                             ^
>   /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: note: put the semicolon on a separate line to silence this warning
>
> Putting the semicolon on its own line is not a big sacrifice to get rid of this
> warning.  I think it's also useful to keep this, because it can catch errors
> like this:
>
>   while (something);
>     {
>       ...
>     }
>
> although gcc would warn about it in a different way (misleading indentation).
>
> This warning is already discussed here in the GCC bugzilla:
>
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62184
>
> gdb/ChangeLog:
>
> 	* dtrace-probe.c (dtrace_process_dof_probe): Put semi-colon on
> 	its own line.
> ---
>  gdb/dtrace-probe.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
> index 122f8de..9a02694 100644
> --- a/gdb/dtrace-probe.c
> +++ b/gdb/dtrace-probe.c
> @@ -421,7 +421,8 @@ dtrace_process_dof_probe (struct objfile *objfile,
>  	  arg.type_str = xstrdup (p);
>  
>  	  /* Use strtab_size as a sentinel.  */
> -	  while (*p++ != '\0' && p - strtab < strtab_size);
> +	  while (*p++ != '\0' && p - strtab < strtab_size)
> +	    ;  /* Silence clang's -Wempty-body warning.  */

Lately I've been choosing to explicitly put "continue;" when the body
doesn't contain anything, like:

	while (*p++ != '\0' && p - strtab < strtab_size)
	  continue;

I don't know what others think about it, but it would solve this problem
and also be more verbose that we're just iterating without a body.

>  
>  	  /* Try to parse a type expression from the type string.  If
>  	     this does not work then we set the type to `long
> -- 
> 2.7.4
Simon Marchi June 21, 2017, 9:08 p.m. UTC | #2 
On 2017-06-21 22:34, Sergio Durigan Junior wrote:
> On Wednesday, June 21 2017, Simon Marchi wrote:
> 
>> clang shows this warning.
>> 
>>   /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: error: 
>> while loop has empty body [-Werror,-Wempty-body]
>>             while (*p++ != '\0' && p - strtab < strtab_size);
>>                                                             ^
>>   /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: note: put 
>> the semicolon on a separate line to silence this warning
>> 
>> Putting the semicolon on its own line is not a big sacrifice to get 
>> rid of this
>> warning.  I think it's also useful to keep this, because it can catch 
>> errors
>> like this:
>> 
>>   while (something);
>>     {
>>       ...
>>     }
>> 
>> although gcc would warn about it in a different way (misleading 
>> indentation).
>> 
>> This warning is already discussed here in the GCC bugzilla:
>> 
>>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62184
>> 
>> gdb/ChangeLog:
>> 
>> 	* dtrace-probe.c (dtrace_process_dof_probe): Put semi-colon on
>> 	its own line.
>> ---
>>  gdb/dtrace-probe.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
>> index 122f8de..9a02694 100644
>> --- a/gdb/dtrace-probe.c
>> +++ b/gdb/dtrace-probe.c
>> @@ -421,7 +421,8 @@ dtrace_process_dof_probe (struct objfile *objfile,
>>  	  arg.type_str = xstrdup (p);
>> 
>>  	  /* Use strtab_size as a sentinel.  */
>> -	  while (*p++ != '\0' && p - strtab < strtab_size);
>> +	  while (*p++ != '\0' && p - strtab < strtab_size)
>> +	    ;  /* Silence clang's -Wempty-body warning.  */
> 
> Lately I've been choosing to explicitly put "continue;" when the body
> doesn't contain anything, like:
> 
> 	while (*p++ != '\0' && p - strtab < strtab_size)
> 	  continue;
> 
> I don't know what others think about it, but it would solve this 
> problem
> and also be more verbose that we're just iterating without a body.

I also looks good, but I don't have a preference.  I'll do that if 
others like it too.

Thanks,

Simon
Pedro Alves June 21, 2017, 9:36 p.m. UTC | #3 
On 06/21/2017 09:15 PM, Simon Marchi wrote:
>  	  /* Use strtab_size as a sentinel.  */
> -	  while (*p++ != '\0' && p - strtab < strtab_size);
> +	  while (*p++ != '\0' && p - strtab < strtab_size)
> +	    ;  /* Silence clang's -Wempty-body warning.  */

I'd must put the ; on its own line (there's probably something
in the coding conventions about this already), and without the
comment.  It's quite common to write for/while loop like that,
see e.g.,:

  $ grep "^[ |\t]*;[ |\t]*$" *.c -C 2

Sure the comment makes sense in the context of the patch,
but when reading the code without considering the patch's context,
it just looks like noise to me.

Thanks,
Pedro Alves
Simon Marchi June 25, 2017, 10:48 a.m. UTC | #4 
On 2017-06-21 23:36, Pedro Alves wrote:
> On 06/21/2017 09:15 PM, Simon Marchi wrote:
>>  	  /* Use strtab_size as a sentinel.  */
>> -	  while (*p++ != '\0' && p - strtab < strtab_size);
>> +	  while (*p++ != '\0' && p - strtab < strtab_size)
>> +	    ;  /* Silence clang's -Wempty-body warning.  */
> 
> I'd must put the ; on its own line (there's probably something
> in the coding conventions about this already), and without the
> comment.  It's quite common to write for/while loop like that,
> see e.g.,:
> 
>   $ grep "^[ |\t]*;[ |\t]*$" *.c -C 2
> 
> Sure the comment makes sense in the context of the patch,
> but when reading the code without considering the patch's context,
> it just looks like noise to me.

Ok, since that's already a common practice in our codebase, I added it 
to our wiki (I didn't find anything related to that in the GNU 
standards):

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#preview

I am pushing without the comment.

Thanks,

Simon
Simon Marchi June 25, 2017, 10:57 a.m. UTC | #5 
On 2017-06-25 12:48, Simon Marchi wrote:
> Ok, since that's already a common practice in our codebase, I added it
> to our wiki (I didn't find anything related to that in the GNU
> standards):
> 
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#preview

Sorry, partly wrong URL:

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Loop_with_empty_body
diff mbox

Patch

diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
index 122f8de..9a02694 100644
--- a/gdb/dtrace-probe.c
+++ b/gdb/dtrace-probe.c
@@ -421,7 +421,8 @@  dtrace_process_dof_probe (struct objfile *objfile,
 	  arg.type_str = xstrdup (p);
 
 	  /* Use strtab_size as a sentinel.  */
-	  while (*p++ != '\0' && p - strtab < strtab_size);
+	  while (*p++ != '\0' && p - strtab < strtab_size)
+	    ;  /* Silence clang's -Wempty-body warning.  */
 
 	  /* Try to parse a type expression from the type string.  If
 	     this does not work then we set the type to `long