Re: [PATCH 1/3] Implement -Wmisleading-indentation (v4)

[Jeff Law <law at redhat dot com>]

On 04/28/2015 06:02 PM, David Malcolm wrote:
> This is an updated implementation of the proposed
> -Wmisleading-indentation warning.
>
> Changes since last patch:
>
> * I've rewritten it to use Manuel's approach from
>      https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01225.html
>    I took Manuel's proposed patch for the C frontend and generalized
>    it to also work on the C++ frontend.  I moved the logic into a new
>    file: c-family/c-indentation.c.
>
> * I've consolidated the testcases into 3 files.
>
> * I've added the option to -Wall, and added enough smarts to the
>    code so that it can bootstrap our own source tree without any false
>    positives.  See the testcases for examples of what I ran into.
I'm very hesitant of enabling this with -Wall without seeing what it 
does on other codebases, particularly those which are not based in a GNU 
formatting style.

For example, what does it to with KNF that's highly used in the BSD 
world or the  Linux kernel style?  I suspect we'll do the right thing 
because the major difference isn't whether or not to indent, but where 
the braces are put.  But some testing on the BSD or Linux kernel would 
go a long way to making me more comfortable with -Wall inclusion.

Also note that a bootstrap doesn't test C all that well anymore since 
GCC itself is mostly compiled with a C++ compiler.  But we're probably 
getting enough coverage to ensure we haven't totally mucked up sometihng 
in the runtime libraries.


> The only remaining known wart here is in the new testcase
> c-c++-common/Wmisleading-indentation-2.c, which uses a #line
> directive to refer to a .md file.  Ideally it ought to be able to
> actually locate the .md file during compilation, since that case is
> trickier to handle than a simple file-not-found.  Hence in theory we
> could have some logic in a .exp to handle copying up the .md from the
> srcdir to the testdir.  That said, the behavior is outwardly
> invisible in both cases: a false positive is not emitted.
Also bear in mind that the GCC testsuite is support to be able to be run 
on an installed tree.  Which brings up the larger issue, namely that 
whatever file generate the #line statements may not be available at the 
time you actually compile the code.  At best you could go look for it 
and use it if found and fallback to some sensible behaviour if not found.


I wonder if some of the cases where you're not working, such as

if (...)
bar;
com;

Ought to be warned for using various -Wmisleading-indentation=X options.

Your call if you want to tackle that in the future.


> gcc/ChangeLog:
> 	* Makefile.in (C_COMMON_OBJS): Add c-family/c-indentation.o.
>
> gcc/c-family/ChangeLog:
> 	* c-common.h (warn_for_misleading_indentation): New prototype.
> 	* c-indentation.c: New file.
> 	* c.opt (Wmisleading-indentation): New option.
>
> gcc/c/ChangeLog:
> 	* c-parser.c (c_parser_if_body): Add param "if_loc", use it
> 	to add a call to warn_for_misleading_indentation.
> 	(c_parser_else_body): Likewise, adding param "else_loc".
> 	(c_parser_if_statement): Check for misleading indentation.
> 	(c_parser_while_statement): Likewise.
> 	(c_parser_for_statement): Likewise.
>
> gcc/cp/ChangeLog:
> 	* parser.c (cp_parser_selection_statement): Add location and
> 	guard_kind arguments to calls to
> 	cp_parser_implicitly_scoped_statement.
> 	(cp_parser_iteration_statement): Likewise for calls to
> 	cp_parser_already_scoped_statement.
> 	(cp_parser_implicitly_scoped_statement): Add "guard_loc" and
> 	"guard_kind" params; use them to warn for misleading
> 	indentation.
> 	(cp_parser_already_scoped_statement): Likewise.
>
> gcc/ChangeLog:
> 	* doc/invoke.texi (Warning Options): Add -Wmisleading-indentation.
> 	(-Wall): Add -Wmisleading-indentation.
> 	(-Wmisleading-indentation): New option.
>
> gcc/testsuite/ChangeLog:
> 	* c-c++-common/Wmisleading-indentation.c: New testcase.
> 	* c-c++-common/Wmisleading-indentation-2.c: New testcase.
> 	* c-c++-common/Wmisleading-indentation-2.md: New file.
>
> libcpp/ChangeLog:
> 	* directives.c (do_line): Set seen_line_directive on line_table.
> 	(do_linemarker): Likewise.
> 	* include/line-map.h (struct line_maps): Add new field
> 	"seen_line_directive".
OK if you take it out of -Wall for now.  We can revisit it in -Wall 
separately :-)

jeff