[PATCH] PR15614 : -frewrite-includes causes -Wunused-macros false positives #2

[Richard Smith]

On Thu, May 1, 2014 at 8:07 AM, Lubos Lunak <l.lunak at centrum.cz> wrote:

> On Sunday 13 of April 2014, Richard Smith wrote:
> > On Fri, Apr 4, 2014 at 3:47 AM, Lubos Lunak <l.lunak at centrum.cz> wrote:
> > >  This is a re-send of
> > >
> > >
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131125/09423
> > >1.html which went without any answer.
> > >
> > >  See PR15614 for a testcase.
> >
> > The combination of -Wunused-macro and -frewrite-includes still doesn't
> > really work with this patch applied: if the only use of a macro is
> within a
> > preprocessor directive that -frewrite-includes expands, we'll still warn
> > about it. Unfortunately I don't have any great ideas as to how to solve
> > this; the least bad idea I have is to add a '#pragma clang macro_used
> > BLAH', and emit that for each macro that's used in the -frewrite-includes
> > run.
>
>  But that's not directly related to this patch, is it? The patch fixes a
> relatively common case, and even if your rare case remains, the patch is
> still an improvement.


Just so you understand my thinking here: we try to avoid applying temporary
partial fixes that we know are going in the wrong direction. In order to
judge whether this is going in the right direction, we need to have an idea
of what the long-term strategy is, so it is relevant to discuss that in the
context of this patch.

If we could get this warning to work with -frewrite-includes by checking
for macro uses during the rewriting step, then this patch would be a step
in the wrong direction (and maybe that's what Eli was thinking of in his
comment on the previous review thread). But I don't think that's viable,
since a macro name could be formed by a token paste or similar, so I think
the /only/ reasonable approach is to only diagnose -Wunused-macro when
compiling and not when performing -frewrite-includes (with some kind of
mechanism to indicate to the compilation that the rewriting step used a
macro).

On that basis, I'm happy with the direction. I'm a bit uncomfortable about
the patch itself, though: restoring the macro expansion flag early in
HandleDirective (so that HandleDefineDirective can tell we're in
rewrite-includes mode) seems like a hack to me. (As an example of why this
seems like the wrong place for this change, it would do the wrong thing if
we had any kind of extension that performed any macro expansion within a
#define directive.)

Maybe rename MacroExpansionInDirectivesOverride to
MacroExpansionOnlyInDirectives or similar, and use that from
HandleDefineDirective?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140501/00d58516/attachment.html>