<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 1, 2014 at 8:07 AM, Lubos Lunak <span dir="ltr"><<a href="mailto:l.lunak@centrum.cz" target="_blank">l.lunak@centrum.cz</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Sunday 13 of April 2014, Richard Smith wrote:<br>
> On Fri, Apr 4, 2014 at 3:47 AM, Lubos Lunak <<a href="mailto:l.lunak@centrum.cz">l.lunak@centrum.cz</a>> wrote:<br>
> >  This is a re-send of<br>
> ><br>
> > <a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131125/09423" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131125/09423</a><br>
</div>> >1.html which went without any answer.<br>
<div class="">> ><br>
> >  See PR15614 for a testcase.<br>
><br>
> The combination of -Wunused-macro and -frewrite-includes still doesn't<br>
> really work with this patch applied: if the only use of a macro is within a<br>
> preprocessor directive that -frewrite-includes expands, we'll still warn<br>
> about it. Unfortunately I don't have any great ideas as to how to solve<br>
> this; the least bad idea I have is to add a '#pragma clang macro_used<br>
> BLAH', and emit that for each macro that's used in the -frewrite-includes<br>
> run.<br>
<br>
</div> But that's not directly related to this patch, is it? The patch fixes a<br>
relatively common case, and even if your rare case remains, the patch is<br>
still an improvement.</blockquote><div><br></div><div>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.</div>
<div><br></div><div>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).</div>
<div><br></div><div>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.)</div>
<div><br></div><div>Maybe rename MacroExpansionInDirectivesOverride to MacroExpansionOnlyInDirectives or similar, and use that from HandleDefineDirective?</div></div></div></div>