On Tue, 2020-07-28 at 20:22 -0400, Patrick Palka wrote:
> On Tue, 28 Jul 2020, David Malcolm wrote:
>
> > On Tue, 2020-07-28 at 15:50 -0400, Patrick Palka wrote:
> > > Currently the -Wmisleading-indentation warning doesn't do any
> > > analysis
> > > when the guarded statement or the statement after it is produced
> > > by a
> > > macro, as the mentioned PR illustrates. This means that we warn
> > > for:
> > >
> > > if (flag)
> > > foo ();
> > > bar ();
> > >
> > > and yet we don't warn for:
> > >
> > > #define BAR bar
> > > if (flag)
> > > foo ();
> > > BAR ();
> > >
> > > which seems like a surprising limitation.
> >
> > IIRC we were trying to keep things simple in the initial
> > implementation.
> >
> > > This patch extends the -Wmisleading-indentation implementation to
> > > support analyzing such statements and their tokens. This is done
> > > in
> > > the
> > > "natural" way by resolving the location of each of the three
> > > tokens
> > > to
> > > the token's macro expansion point. (Additionally, if the tokens
> > > all
> > > resolve to the same macro expansion point then we instead use
> > > their
> > > locations within the macro definition.) When these resolved
> > > locations
> > > are all different, then we can proceed with applying the warning
> > > heuristics to them as if no macros were involved.
> >
> > Thanks for working on this. I've only looked at the patch briefly,
> > but
> > so far it looks reasonable.
> >
> > Can you post some examples of what the output looks like for some
> > of
> > these cases? The diagnostics code has some logic for printing how
> > macros are unwound, so I'm wondering what we actually print for
> > these
> > cases, and if it looks intelligble to an end-user.
>
> For the code fragment mentioned above:
>
> int
> main()
> {
> #define BAR
> if (flag)
> foo ();
> BAR ();
> }
>
> we emit:
>
> test.cc: In function ‘int main()’:
> test.cc:7:3: warning: this ‘if’ clause does not guard... [-
> Wmisleading-indentation]
> 9 | if (flag)
> | ^~
> test.cc:8:15: note: ...this statement, but the latter is misleadingly
> indented as if it were guarded by the ‘if’
> 8 | #define BAR bar
> | ^~~
> test.cc:10:5: note: in expansion of macro ‘BAR’
> 11 | BAR ();
> | ^~~
>
>
> And when all tokens are generated from the same macro, e.g. for:
>
> int
> main()
> {
> #define BAR bar
> #define DO_STUFF \
> if (flag) \
> foo (); \
> BAR ();
> DO_STUFF;
> }
>
> we emit:
>
> test.cc: In function ‘int main()’:
> test.cc:11:5: warning: this ‘if’ clause does not guard... [-
> Wmisleading-indentation]
> 11 | if (flag) \
> | ^~
> test.cc:14:3: note: in expansion of macro ‘DO_STUFF’
> 14 | DO_STUFF;
> | ^~~~~~~~
> test.cc:9:15: note: ...this statement, but the latter is misleadingly
> indented as if it were guarded by the ‘if’
> 9 | #define BAR bar
> | ^~~
> test.cc:13:7: note: in expansion of macro ‘BAR’
> 13 | BAR ();
> | ^~~
> test.cc:14:3: note: in expansion of macro ‘DO_STUFF’
> 14 | DO_STUFF;
> | ^~~~~~~~
>
> The automatic macro unwinding logic saves the day here and yields
> mostly
> legible output "for free". What do you think?
Sorry about the delay in responding.
I agree that the output is "mostly legible".
The patch is OK as-is, and looking at the fixes you made to our source
tree I think it's usefully catching problematic code.
That said, I wonder if the output could be improved for the macro case.
Within should_warn_for_misleading_indentation the patch can update
guard_loc, body_loc and next_stmt_loc before expand_location is called
on them, but then in warn_for_misleading_indentation it uses the
original guard_tinfo.location and next_tinfo.location for the locations
of the warning and note. I have a hunch that the readability of the
diagnostic would be improved for the macro case by updating those
locations whenever the inputs to expand_location are updated. It would
lose some detail from the point of view of the user grokking macro
expansions, but perhaps is better given the specific focus of this
warning on whitespace. Does doing so help for the various cases you've
tried?
Thanks
Dave
