This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Please, take '-Wmisleading-indentation' out of -Wall


On Wed, 2016-05-04 at 18:15 +0200, Antonio Diaz Diaz wrote:
> [Please, CC me. I'm not subscribed to gcc@gcc.gnu.org].
> 
> First of all, thank you very much for gcc.
> 
> I am not an expert in gcc. Please, forgive any mistakes in this
> message. :-)
> 
> After compiling all my projects with gcc-6.1, I have received
> warnings 
> related to -Wmisleading-indentation in four of them for constructs
> that 
> I do not consider questionable. (Mainly one-liners inside switch 
> statements with no indentation involved). As it is now, the name of
> the 
> option is misleading, because -Wmisleading-indentation does not limit
> itself to warn about misleading indentation.
> 	
> One of the warnings is a false positive for the following code from
> GNU 
> ed (the 'break' is aligned with the 'while', all spaces, no tabs, yet
> gcc complains that it is misleadingly indented):
>      case '#': while( *(*ibufpp)++ != '\n' ) ;
>                break;

I agree that this is a false positive.  I downloaded ed-1.13 and tried
it with a recent build of gcc and I also see it, in main_loop.c.

I've filed a bug about this here:

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

(aka "PR c/70954") and I'll have a look at fixing it.

Sorry about the bug.

Does that bug fully capture the kind of issue you're seeing, or are
there other kinds of false positives?  (I only see that single false
positive in ed-1.13, but you mentioned four projects in total).


> I consider -Wmisleading-indentation an useful addition to gcc, but
> IMO 
> there are a number of reasons that make its inclusion in -Wall 
> inappropriate or at least excessively annoying.
> 
> To begin with, the C and C++ standards state clearly that the amount
> of 
> whitespace characters separating tokens is not significant.
> Therefore, 
> warning about indentation in C/C++ must be considered optional. More 
> optional than -Woverlength-strings, which is not in -Wall
> nor in -Wextra.
> 
> C/C++ allows creative use of whitespace to highlight the estructure
> of 
> the code in ways that -Wmisleading-indentation can't anticipate. 

Warning about indentation is gcc *is* optional, that's why you have to
enable -Wall or -Wmisleading-indentation to get it.

Re "-Woverlength-strings", I don't think that there's a single scale of
"optionalness" against which warnings can be considered.

> Including -Wmisleading-indentation in -Wall forces the developer to 
> surrender such freedom just to placate gcc:
> 
> http://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.htm
> l
> "Don't make the program ugly just to placate static analysis tools
> such
> as 'lint', 'clang', and GCC with extra warnings options such as
> '-Wconversion' and '-Wundef'. These tools can help find bugs and
> unclear
> code, but they can also generate so many false alarms that it hurts
> readability to silence them with unnecessary casts, wrappers, and
> other
> complications. For example, please don't insert casts to 'void' or
> calls
> to do-nothing functions merely to pacify a lint checker".

FWIW I think the issue here is that the warning was confused by the
"case '#':" on the same line as the "while"); I can suppress the
warning (in ed-1.13) via the following addition of a comment:

--- main_loop.c.orig    2016-05-04 14:10:05.263880101 -0400
+++ main_loop.c 2016-05-04 14:11:55.741347000 -0400
@@ -622,7 +622,8 @@ static int exec_command( const char ** c
                   !display_lines( second_addr, second_addr, 0 ) )
                 return ERR;
               break;
-    case '#': while( *(*ibufpp)++ != '\n' ) ;
+    case '#': /* Skip comment.  */
+              while( *(*ibufpp)++ != '\n' ) ;
               break;
     default : set_error_msg( "Unknown command" ); return ERR;
     }

though, as I said, it's a bug.

> I also think that -Wmisleading-indentation should not be enabled by 
> -Wall nor -Wextra because:
>    - -Wall is defined in the gcc manual as enabling all the warnings
>      about constructions that are easy to avoid (or modify to prevent
> the
>      warning), even in conjunction with macros. This is not true of
>      -Wmisleading-indentation.

It's trivial to work around, by changing the indentation.  What we have
here is a bug.  Admittedly it took me a few tries to work out an
indentation that the warning was happy with, but I hope to fix that as
part of the above bug fix.

>    - It can't be portably disabled; older versions of gcc do not
> accept
>      '-Wno-misleading-indentation'. (At least 4.1.2 does not accept
> it).

FWIW "-Wall -Wno-misleading-indentation" works for me with gcc 4.8.3

>    - If there are no macros involved, it is independent of code
>      generation; once tested in the developer's machine there is no
> need
>      to test it again on every user's machine.

I don't understand this objection.

>    - It makes compilation a 0.5-1% slower for every user for no
> reason.

Where did these numbers come from?

>    - -Wempty-body is much simpler to test for, and in general less
>      questionable than -Wmisleading-indentation, yet it is not
> enabled by
>      -Wall.
> 
> I think that keeping separated categories of warnings (instead of 
> warning about everything by default) is a valuable feature. Maybe
> both 
> -Wempty-body and -Wmisleading-indentation (and any future similar 
> options) could be put in a new category (-Wcoding-style or 
> -Wstatic-analysis, for example).
> 
> To summarize, I want 'gcc -Wall -Wextra' to:
>    - not warn about the use of whitespace beyond what the standard
>      mandates.
>    - not warn about the format of one-liners.
>    - not being slowed down because of indentation parsing.
> 
> Please, take '-Wmisleading-indentation' out of -Wall (and don't move
> it 
> to -Wextra).
> 
> 
> Thanks in advance,
> Antonio.

Thanks, I hope this is a constructive response.
Dave


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]