cor3ntin added inline comments.
================ Comment at: clang/lib/Parse/ParseExpr.cpp:3288-3291 + assert( + (StringToks.size() != 1 || + !tok::isUnexpandableMsMacro(StringToks[0].getKind())) && + "single predefined identifiers shall be handled by ActOnPredefinedExpr"); ---------------- RIscRIpt wrote: > tahonermann wrote: > > What is the reason for requiring non-concatenated predefined function local > > macros to be handled by `ActOnPredefinedExpr()`? > To ensure generation of the same AST as before my changes (with > `PredefinedExpr`): > > ``` > | | `-VarDecl <col:2, col:19> col:13 a 'const char[2]' cinit > | | `-PredefinedExpr <col:19> 'const char[2]' __FUNCTION__ > | | `-StringLiteral <col:19> 'const char[2]' "f" > ``` > > This actually brings a question: do we want to wrap StringLiterals (into > PredefinedExpr) which were formed from concatenation of string-literals and > some predefined identifier(s)? But it does not really make any sense: > concatenated string does not represent any of PredefinedExpr. Yes, this make sense. I think it's better than adding more complexity / memory footprint to `StringLiteral`. ================ Comment at: clang/lib/Parse/ParseExpr.cpp:3294-3298 StringToks.push_back(Tok); - ConsumeStringToken(); - } while (isTokenStringLiteral()); + if (isTokenStringLiteral()) + ConsumeStringToken(); + else + ConsumeToken(); ---------------- RIscRIpt wrote: > cor3ntin wrote: > > I think I'd prefer `ConsumeAnyToken` with an assert that checks it's a > > stringLiteral or a predefined ms exppression > Done. But do we really need an assertion here? We have one in the function > preamble and strict condition in `while`. Looking at that again, i think you can remove the assert (the one at the top should stay). which you did, excellent. sorry about that! ================ Comment at: clang/lib/Sema/SemaExpr.cpp:1955-1991 + // MSVC treats some of predefined identifiers (e.g. __FUNCTION__) as + // expandable predefined macros defined as string literals, + // which may be concatenated. Expand them here (in Sema), + // because StringLiteralParser (in Lex) doesn't have access to AST. + std::vector<Token> ExpandedToks; + if (getLangOpts().MicrosoftExt) { + ExpandedToks = StringToks.vec(); ---------------- RIscRIpt wrote: > cor3ntin wrote: > > Can we put that logic in a separate function? > Done. Tho, I couldn't make it `const` (for the same reason I couldn't make > `getCurScopeDecl() const`). And I wasn't sure about the interface: > ``` > std::vector<Token> ExpandMSPredefinedMacros(ArrayRef<Token> Toks); > ``` > vs > ``` > void ExpandMSPredefinedMacros(MutableArrayRef<Token> Toks); > ``` I think the later let you use a small vector so it's probably more efficient. I'm find with either ================ Comment at: clang/lib/Sema/SemaExpr.cpp:1972-1987 + SmallString<64> Str; + llvm::raw_svector_ostream OS(Str); + Token Exp; + Exp.startToken(); + if (Tok.getKind() == tok::kw_L__FUNCTION__ || + Tok.getKind() == tok::kw_L__FUNCSIG__) { + OS << 'L'; ---------------- RIscRIpt wrote: > cor3ntin wrote: > > I think it might be easier to create a string_literal token directly here. > > I'm also not sure we need to use `Lexer::Stringify` > > I think it might be easier to create a string_literal token directly here. > > What do you mean? Is there a function which creates Token object from > StringRef? Or is there a way to associate string literal value with a Token > without PP? I would like to simplify it, but I haven't found other ways of > achieving the same result. > > > I'm also not sure we need to use Lexer::Stringify > > Well, as far as I can see `StringLiteralParser` expands escape sequences. So, > I am just being too careful here. > If not using `Lexer::Stringify` do we want to assert that function name does > not contain neither `"` not `\` (which should not happen™)? Thanks! I really would get rid of `Stringify` here - I struggle to see how a function could be produced that has characters that cannot appear in an identifier. even asserting isn't very useful. ================ Comment at: clang/test/Sema/ms_predefined_expr.cpp:9 const char e[] = __PRETTY_FUNCTION__; // expected-warning{{initializing an array from a '__PRETTY_FUNCTION__' predefined identifier is a Microsoft extension}} + const wchar_t f[] = L__FUNCTION__; // expected-warning{{initializing an array from a 'L__FUNCTION__' predefined identifier is a Microsoft extension}} + const wchar_t g[] = L__FUNCSIG__; // expected-warning{{initializing an array from a 'L__FUNCSIG__' predefined identifier is a Microsoft extension}} ---------------- RIscRIpt wrote: > cor3ntin wrote: > > do we have any tests that look at the values of these things? > ``` > clang/test/Analysis/eval-predefined-exprs.cpp > clang/test/AST/Interp/literals.cpp > clang/test/SemaCXX/source_location.cpp > clang/test/SemaCXX/predefined-expr.cpp > ``` I think it's worth add a few more tests in clang/test/SemaCXX/predefined-expr.cpp checking concatenations Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153914/new/ https://reviews.llvm.org/D153914 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits