[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
Comment at: clang/include/clang/Parse/Parser.h:578-582
+  bool isTokenConcatenable() const {
+    return isTokenStringLiteral() ||
+           getLangOpts().MicrosoftExt &&
+               tok::isMSPredefinedMacro(Tok.getKind());
+  }
----------------
Unless you find a better name,  I think it's preferable to keep `isTokenConcatenable` and `isTokenPredefinedMsMacro` as separate functions.
Also, this seems like a weird place to check for  `getLangOpts().MicrosoftExt`


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3294-3298
     StringToks.push_back(Tok);
-    ConsumeStringToken();
-  } while (isTokenStringLiteral());
+    if (isTokenStringLiteral())
+      ConsumeStringToken();
+    else
+      ConsumeToken();
----------------
I think I'd prefer `ConsumeAnyToken` with an assert that checks it's a stringLiteral or a predefined ms exppression


================
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();
----------------
Can we put that logic in a separate function?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1964
+    for (Token &Tok : ExpandedToks) {
+      if (!tok::isMSPredefinedMacro(Tok.getKind())) {
+        continue;
----------------
can you assert it's a string literal otherwise?


================
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';
----------------
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`


================
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}}
----------------
do we have any tests that look at the values of these things?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153914/new/

https://reviews.llvm.org/D153914