Fix indentation (and clang warning) in c-lang.c

Message ID 20191216233637.29925-1-simon.marchi@efficios.com
State New, archived
Headers

Commit Message

Simon Marchi Dec. 16, 2019, 11:36 p.m. UTC
  I see this warning when building with clang:

      CXX    c-lang.o
    /home/smarchi/src/binutils-gdb/gdb/c-lang.c:314:7: error: misleading indentation; statement is not part of the previous 'if' [-Werror,-Wmisleading-indentation]
          *length = i * width;
          ^
    /home/smarchi/src/binutils-gdb/gdb/c-lang.c:308:4: note: previous statement is here
              if (extract_unsigned_integer (contents + i * width,
              ^

It took me a while to notice that some lines in that area have a
spurious space before the tabs, at the beginning of the ling.  I'm not
sure how clang translates that to misleading indentation, but making the
indentation correct gets rid of the error.

There are many more instances of this in the code base (`grep -P '^ \t'
*.c`), if others think it's a good idea, it would be pretty easy to fix
them all up in one shot.

gdb/ChangeLog:

	* c-lang.c (c_get_string, asm_language_defn): Remove space
	before tab.
---
 gdb/c-lang.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
  

Comments

Simon Marchi Dec. 16, 2019, 11:50 p.m. UTC | #1
On 2019-12-16 6:36 p.m., Simon Marchi wrote:
> I see this warning when building with clang:

I meant to add the clang version to the commit message, but forgot, I'll
add it locally.  The version is (daily build from apt.llvm.org):

$ clang++-10 --version
clang version 10.0.0-+20191211091425+f99297176cd-1~exp1~20191211082036.1372

Simon
  
Pedro Alves Dec. 17, 2019, 7:24 p.m. UTC | #2
On 12/16/19 11:50 PM, Simon Marchi wrote:
> On 2019-12-16 6:36 p.m., Simon Marchi wrote:
>> I see this warning when building with clang:
> 
> I meant to add the clang version to the commit message, but forgot, I'll
> add it locally.  The version is (daily build from apt.llvm.org):
> 
> $ clang++-10 --version
> clang version 10.0.0-+20191211091425+f99297176cd-1~exp1~20191211082036.1372

Curious.  Are you saying that that changed recently in clang?  I wonder
whether clang's warning catches this case on purpose, or whether it's
a regression.  I asked David Malcolm, who added that warning to GCC, and he
said that GCC's implementation respects tab stops, so small numbers of
leading spaces before a tab are effectively ignored.

Thanks,
Pedro Alves
  
Simon Marchi Dec. 17, 2019, 7:39 p.m. UTC | #3
On 2019-12-17 2:24 p.m., Pedro Alves wrote:
> Curious.  Are you saying that that changed recently in clang?  I wonder
> whether clang's warning catches this case on purpose, or whether it's
> a regression.  I asked David Malcolm, who added that warning to GCC, and he
> said that GCC's implementation respects tab stops, so small numbers of
> leading spaces before a tab are effectively ignored.

The warning appears to be new in clang 10, because it doesn't exist in clang 9:

$ clang++ test.cpp -Wmisleading-indentation
warning: unknown warning option '-Wmisleading-indentation'; did you mean '-Wbinding-in-condition'? [-Wunknown-warning-option]

I can't tell if it's on purpose or not.  I wonder if it's worth opening a bug
on clang's bugzilla to ask whether it's intentional or not, it might help them
tweak the warning before it's officially released...

Simon
  
Pedro Alves Dec. 17, 2019, 7:41 p.m. UTC | #4
On 12/17/19 7:39 PM, Simon Marchi wrote:
> On 2019-12-17 2:24 p.m., Pedro Alves wrote:
>> Curious.  Are you saying that that changed recently in clang?  I wonder
>> whether clang's warning catches this case on purpose, or whether it's
>> a regression.  I asked David Malcolm, who added that warning to GCC, and he
>> said that GCC's implementation respects tab stops, so small numbers of
>> leading spaces before a tab are effectively ignored.
> 
> The warning appears to be new in clang 10, because it doesn't exist in clang 9:
> 
> $ clang++ test.cpp -Wmisleading-indentation
> warning: unknown warning option '-Wmisleading-indentation'; did you mean '-Wbinding-in-condition'? [-Wunknown-warning-option]
> 
> I can't tell if it's on purpose or not.  I wonder if it's worth opening a bug
> on clang's bugzilla to ask whether it's intentional or not, it might help them
> tweak the warning before it's officially released...

Yes, I think it's worth it.  Part of the point of using pre release compilers
is to catch&fix issues before they are released.

Thanks,
Pedro Alves
  
Simon Marchi Dec. 17, 2019, 7:53 p.m. UTC | #5
On 2019-12-17 2:41 p.m., Pedro Alves wrote:
> On 12/17/19 7:39 PM, Simon Marchi wrote:
>> On 2019-12-17 2:24 p.m., Pedro Alves wrote:
>>> Curious.  Are you saying that that changed recently in clang?  I wonder
>>> whether clang's warning catches this case on purpose, or whether it's
>>> a regression.  I asked David Malcolm, who added that warning to GCC, and he
>>> said that GCC's implementation respects tab stops, so small numbers of
>>> leading spaces before a tab are effectively ignored.
>>
>> The warning appears to be new in clang 10, because it doesn't exist in clang 9:
>>
>> $ clang++ test.cpp -Wmisleading-indentation
>> warning: unknown warning option '-Wmisleading-indentation'; did you mean '-Wbinding-in-condition'? [-Wunknown-warning-option]
>>
>> I can't tell if it's on purpose or not.  I wonder if it's worth opening a bug
>> on clang's bugzilla to ask whether it's intentional or not, it might help them
>> tweak the warning before it's officially released...
> 
> Yes, I think it's worth it.  Part of the point of using pre release compilers
> is to catch&fix issues before they are released.

Ok, I opened this:

https://bugs.llvm.org/show_bug.cgi?id=44324

Simon
  
Simon Marchi Dec. 18, 2019, 6:28 p.m. UTC | #6
On 2019-12-16 6:36 p.m., Simon Marchi wrote:
> I see this warning when building with clang:
> 
>       CXX    c-lang.o
>     /home/smarchi/src/binutils-gdb/gdb/c-lang.c:314:7: error: misleading indentation; statement is not part of the previous 'if' [-Werror,-Wmisleading-indentation]
>           *length = i * width;
>           ^
>     /home/smarchi/src/binutils-gdb/gdb/c-lang.c:308:4: note: previous statement is here
>               if (extract_unsigned_integer (contents + i * width,
>               ^
> 
> It took me a while to notice that some lines in that area have a
> spurious space before the tabs, at the beginning of the ling.  I'm not
> sure how clang translates that to misleading indentation, but making the
> indentation correct gets rid of the error.
> 
> There are many more instances of this in the code base (`grep -P '^ \t'
> *.c`), if others think it's a good idea, it would be pretty easy to fix
> them all up in one shot.
> 
> gdb/ChangeLog:
> 
> 	* c-lang.c (c_get_string, asm_language_defn): Remove space
> 	before tab.
> ---
>  gdb/c-lang.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/c-lang.c b/gdb/c-lang.c
> index 846970af7b92..1f40e885d99a 100644
> --- a/gdb/c-lang.c
> +++ b/gdb/c-lang.c
> @@ -303,14 +303,14 @@ c_get_string (struct value *value, gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
>        if (*length >= 0)
>  	i  = *length;
>        else
> - 	/* Otherwise, look for a null character.  */
> - 	for (i = 0; i < fetchlimit; i++)
> +	/* Otherwise, look for a null character.  */
> +	for (i = 0; i < fetchlimit; i++)
>  	  if (extract_unsigned_integer (contents + i * width,
>  					width, byte_order) == 0)
> - 	    break;
> +	    break;
>    
>        /* I is now either a user-defined length, the number of non-null
> - 	 characters, or FETCHLIMIT.  */
> +	 characters, or FETCHLIMIT.  */
>        *length = i * width;
>        buffer->reset ((gdb_byte *) xmalloc (*length));
>        memcpy (buffer->get (), contents, *length);
> @@ -1119,7 +1119,7 @@ extern const struct language_defn asm_language_defn =
>    0,				/* String lower bound */
>    default_word_break_characters,
>    default_collect_symbol_completion_matches,
> -  c_language_arch_info, 	/* FIXME: la_language_arch_info.  */
> +  c_language_arch_info,		/* FIXME: la_language_arch_info.  */
>    default_print_array_index,
>    default_pass_by_reference,
>    c_watch_location_expression,
> -- 
> 2.24.1
> 

I pushed this one.

Simon
  

Patch

diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index 846970af7b92..1f40e885d99a 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -303,14 +303,14 @@  c_get_string (struct value *value, gdb::unique_xmalloc_ptr<gdb_byte> *buffer,
       if (*length >= 0)
 	i  = *length;
       else
- 	/* Otherwise, look for a null character.  */
- 	for (i = 0; i < fetchlimit; i++)
+	/* Otherwise, look for a null character.  */
+	for (i = 0; i < fetchlimit; i++)
 	  if (extract_unsigned_integer (contents + i * width,
 					width, byte_order) == 0)
- 	    break;
+	    break;
   
       /* I is now either a user-defined length, the number of non-null
- 	 characters, or FETCHLIMIT.  */
+	 characters, or FETCHLIMIT.  */
       *length = i * width;
       buffer->reset ((gdb_byte *) xmalloc (*length));
       memcpy (buffer->get (), contents, *length);
@@ -1119,7 +1119,7 @@  extern const struct language_defn asm_language_defn =
   0,				/* String lower bound */
   default_word_break_characters,
   default_collect_symbol_completion_matches,
-  c_language_arch_info, 	/* FIXME: la_language_arch_info.  */
+  c_language_arch_info,		/* FIXME: la_language_arch_info.  */
   default_print_array_index,
   default_pass_by_reference,
   c_watch_location_expression,