r173952 - [preprocessor] Don't warn about "disabled expansion of recursive macro"

[Douglas Gregor]

On Jan 30, 2013, at 2:21 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Wed, Jan 30, 2013 at 2:16 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Wed, Jan 30, 2013 at 1:39 PM, Douglas Gregor <dgregor at apple.com> wrote:
>>> 
>>> On Jan 30, 2013, at 1:23 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>> 
>>>> On Wed, Jan 30, 2013 at 12:44 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>>>> On Jan 30, 2013, at 11:54 AM, Abramo Bagnara <abramo.bagnara at gmail.com> wrote:
>>>>> 
>>>>>> Il 30/01/2013 19:55, Argyrios Kyrtzidis ha scritto:
>>>>>>> Author: akirtzidis
>>>>>>> Date: Wed Jan 30 12:55:52 2013
>>>>>>> New Revision: 173952
>>>>>>> 
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=173952&view=rev
>>>>>>> Log:
>>>>>>> [preprocessor] Don't warn about "disabled expansion of recursive macro"
>>>>>>> for "#define X X".
>>>>>>> 
>>>>>>> This is a pattern that, for example, stdbool.h uses.
>>>>>>> rdar://12435773
>>>>>> 
>>>>>> Please revert this commit: this has already been discussed in detail in
>>>>>> past and we decided otherwise. Last time it has been discussed in this
>>>>>> thread:
>>>>>> 
>>>>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20121029/067098.html
>>>>> 
>>>>> Reverted in r173970.
>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>> Modified:
>>>>>>>  cfe/trunk/lib/Lex/PPMacroExpansion.cpp
>>>>>>>  cfe/trunk/test/Headers/stdbool.cpp
>>>>>>>  cfe/trunk/test/Preprocessor/warn-disabled-macro-expansion.c
>>>>>>> 
>>>>>>> Modified: cfe/trunk/lib/Lex/PPMacroExpansion.cpp
>>>>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPMacroExpansion.cpp?rev=173952&r1=173951&r2=173952&view=diff
>>>>>>> ==============================================================================
>>>>>>> --- cfe/trunk/lib/Lex/PPMacroExpansion.cpp (original)
>>>>>>> +++ cfe/trunk/lib/Lex/PPMacroExpansion.cpp Wed Jan 30 12:55:52 2013
>>>>>>> @@ -459,7 +459,10 @@ bool Preprocessor::HandleMacroExpandedId
>>>>>>>     if (MacroInfo *NewMI = getMacroInfo(NewII))
>>>>>>>       if (!NewMI->isEnabled() || NewMI == MI) {
>>>>>>>         Identifier.setFlag(Token::DisableExpand);
>>>>>>> -          Diag(Identifier, diag::pp_disabled_macro_expansion);
>>>>>>> +          // Don't warn for "#define X X" like "#define bool bool" from
>>>>>>> +          // stdbool.h.
>>>>>> 
>>>>>> Would you be so kind to add a comment here to remember that the choice
>>>>>> to have this warning for thar case (although disabled by default) is
>>>>>> deliberate?
>>>>> 
>>>>> Just to make sure, are you of the opinion that we should warn when <stdbool.h> is included and its macros are used ?
>>>> 
>>>> As discussed on that thread, we should modify <stdbool.h> to suppress
>>>> the warning.
>>> 
>>> How do you propose to do that? #pragmas don't help, because the diagnostic comes from the expansion location, not the definition location. Changing #define bool bool to something that involves _Pragma is very likely to break code elsewhere.
>> 
>> To my reading of C11 6.10.3.4, that's a bug in Clang's preprocessor.
>> _Pragmas are supposed to take effect (and, per 6.10.9/1, be removed
>> from the token stream) during macro rescanning, so I think this should
>> work:
>> 
>> #define true _Pragma("clang diagnostic push") _Pragma("clang
>> diagnostic ignored \"-Wdisabled-macro-expansion\"") true
>> _Pragma("clang diagnostic pop")
>> 
>> #define CAT2(x,y) x##y
>> #define CAT(x,y) CAT2(x,y)
>> bool b = CAT(true,n);
>> 
>> ... because the pragma should take effect during the rescan of the
>> expansion of 'true', before we perform the token paste.
>> 
>> Another example we seem to get wrong:
>> 
>> #define foo _Pragma("GCC diagnostic ignored \"-Wuninitialized\"")
>> #define str(x) #x
>> #define STR(x) str(x)
>> const char *p = STR(foo);
>> int n() { int k; return k; }
>> 
>> This should produce an empty string and no warning, as far as I can tell.
> 
> FWIW, gcc gets this right, producing an empty string and suppressing
> the warning here:
> 
> #define foo _Pragma("GCC diagnostic ignored \"-Wuninitialized\"")
> #define str(x) #x
> #define STR(x) str(x)
> void f() {
> const char *p = ({
> STR(foo);
> });
> }
> int n() { int k; return k; }
> 
> (extra gunk to get around gcc's restrictions on where pragmas can appear).


Yes, we should fix this. That's still a horrible workaround for a warning that doesn't make sense.

	#define X X

is *not* what this warning is supposed to diagnose. I've reinstated Argyrios' patch in r173991 with a fix to only suppress the warning in this case.

	- Doug