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

[Corentin Jabot via Phabricator via cfe-commits]

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