<div dir="ltr"><div dir="ltr">On Mon, 18 Oct 2021 at 12:56, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Oct 18, 2021 at 3:52 PM Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
><br>
> On Mon, 18 Oct 2021 at 12:48, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>> wrote:<br>
>><br>
>> On Mon, Oct 18, 2021 at 3:33 PM Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
>> ><br>
>> > On Sun, 17 Oct 2021 at 04:58, Aaron Ballman via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >><br>
>> >> Author: Aaron Ballman<br>
>> >> Date: 2021-10-17T07:54:48-04:00<br>
>> >> New Revision: 2edb89c746848c52964537268bf03e7906bf2542<br>
>> >><br>
>> >> URL: <a href="https://github.com/llvm/llvm-project/commit/2edb89c746848c52964537268bf03e7906bf2542" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/2edb89c746848c52964537268bf03e7906bf2542</a><br>
>> >> DIFF: <a href="https://github.com/llvm/llvm-project/commit/2edb89c746848c52964537268bf03e7906bf2542.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/2edb89c746848c52964537268bf03e7906bf2542.diff</a><br>
>> >><br>
>> >> LOG: Lex arguments for __has_cpp_attribute and friends as expanded tokens<br>
>> >><br>
>> >> The C and C++ standards require the argument to __has_cpp_attribute and<br>
>> >> __has_c_attribute to be expanded ([cpp.cond]p5). It would make little sense<br>
>> >> to expand the argument to those operators but not expand the argument to<br>
>> >> __has_attribute and __has_declspec, so those were both also changed in this<br>
>> >> patch.<br>
>> >><br>
>> >> Note that it might make sense for the other builtins to also expand their<br>
>> >> argument, but it wasn't as clear to me whether the behavior would be correct<br>
>> >> there, and so they were left for a future revision.<br>
>> >><br>
>> >> Added:<br>
>> >> clang/test/Preprocessor/has_attribute_errors.cpp<br>
>> >><br>
>> >> Modified:<br>
>> >> clang/docs/ReleaseNotes.rst<br>
>> >> clang/lib/Lex/PPMacroExpansion.cpp<br>
>> >> clang/test/Preprocessor/has_attribute.c<br>
>> >> clang/test/Preprocessor/has_attribute.cpp<br>
>> >> clang/test/Preprocessor/has_c_attribute.c<br>
>> >><br>
>> >> Removed:<br>
>> >><br>
>> >><br>
>> >><br>
>> >> ################################################################################<br>
>> >> diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst<br>
>> >> index 6501a4870e2a6..263eae83036df 100644<br>
>> >> --- a/clang/docs/ReleaseNotes.rst<br>
>> >> +++ b/clang/docs/ReleaseNotes.rst<br>
>> >> @@ -110,6 +110,13 @@ Attribute Changes in Clang<br>
>> >> attribute is handled instead, e.g. in ``handleDeclAttribute``.<br>
>> >> (This was changed in order to better support attributes in code completion).<br>
>> >><br>
>> >> +- __has_cpp_attribute, __has_c_attribute, __has_attribute, and __has_declspec<br>
>> >> + will now macro expand their argument. This causes a change in behavior for<br>
>> >> + code using ``__has_cpp_attribute(__clang__::attr)`` (and same for<br>
>> >> + ``__has_c_attribute``) where it would previously expand to ``0`` for all<br>
>> >> + attributes, but will now issue an error due to the expansion of the<br>
>> >> + predefined ``__clang__`` macro.<br>
>> >> +<br>
>> >> Windows Support<br>
>> >> ---------------<br>
>> >><br>
>> >><br>
>> >> diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp<br>
>> >> index bf19f538647e6..5a0fa5184e38b 100644<br>
>> >> --- a/clang/lib/Lex/PPMacroExpansion.cpp<br>
>> >> +++ b/clang/lib/Lex/PPMacroExpansion.cpp<br>
>> >> @@ -1293,7 +1293,7 @@ static bool EvaluateHasIncludeNext(Token &Tok,<br>
>> >> /// integer values.<br>
>> >> static void EvaluateFeatureLikeBuiltinMacro(llvm::raw_svector_ostream& OS,<br>
>> >> Token &Tok, IdentifierInfo *II,<br>
>> >> - Preprocessor &PP,<br>
>> >> + Preprocessor &PP, bool ExpandArgs,<br>
>> >> llvm::function_ref<<br>
>> >> int(Token &Tok,<br>
>> >> bool &HasLexedNextTok)> Op) {<br>
>> >> @@ -1319,7 +1319,10 @@ static void EvaluateFeatureLikeBuiltinMacro(llvm::raw_svector_ostream& OS,<br>
>> >> bool SuppressDiagnostic = false;<br>
>> >> while (true) {<br>
>> >> // Parse next token.<br>
>> >> - PP.LexUnexpandedToken(Tok);<br>
>> >> + if (ExpandArgs)<br>
>> >> + PP.Lex(Tok);<br>
>> >> + else<br>
>> >> + PP.LexUnexpandedToken(Tok);<br>
>> ><br>
>> ><br>
>> > How does this handle things like:<br>
>> ><br>
>> > #define RPAREN )<br>
>> > #if __has_attribute(clang::fallthrough RPAREN<br>
>> ><br>
>> > ? I think that should be an error: the ) token should not be produced by macro expansion, analogous to the behavior of function-like macros. But I imagine unless we're careful here, we'll allow that macro expansion to terminate the "macro".<br>
>><br>
>> I agree, I think that should be an error. We handle it reasonably too; I get:<br>
>><br>
>> error: missing ')' after 'clang'<br>
>><br>
>> though it appears there is some compiler divergence here:<br>
>> <a href="https://godbolt.org/z/c84cb3PW7" rel="noreferrer" target="_blank">https://godbolt.org/z/c84cb3PW7</a><br>
><br>
><br>
> Oops, I meant __has_cpp_attribute, not __has_attribute. We mishandle that one just like GCC: <a href="https://godbolt.org/z/Ej84Kca16" rel="noreferrer" target="_blank">https://godbolt.org/z/Ej84Kca16</a></blockquote><div><br></div><div><div>Digging a bit more: <a href="https://godbolt.org/z/4Yo578b8n">https://godbolt.org/z/4Yo578b8n</a></div><div><br></div><div>__has_cpp_attribute(clang::FT RPAREN and __has_attribute(FT RPAREN are incorrectly allowed, but __has_cpp_attribute(FT RPAREN is rejected.</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Well that's just neat. I was trying to avoid lexing an unexpanded<br>
token and then expanding it later if it was an identifier, but it<br>
sounds like I should be taking that approach here. (Is that even<br>
possible to do?)<br></blockquote><div><br></div><div>If you want to exactly match macro argument handling, you'll need to first lex unexpanded tokens until you find the `)`, and then expand them separately. Instead, you could ask if the ) token came from macro expansion, and if so, reject, but that will still mishandle cases like</div><div><br></div><div>#define M1 M2(</div><div>#define M2() fallthrough</div><div>__has_cpp_attribute( M1 ) )</div><div><br></div><div>Perhaps we can move the interception of builtin function-like macros to after we prepare to do macro expansion, so we can reuse the existing code for gathering the arguments?</div></div></div>