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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann requested changes to this revision.
tahonermann added a comment.
This revision now requires changes to proceed.

I think this is looking good. The only big thing I noticed is some code that looks like it should have been removed with the last set of changes. Otherwise, just some minor concerns and suggestions.



================
Comment at: clang/lib/Parse/ParseExpr.cpp:1299-1301
+    Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
+    ConsumeToken();
+    break;
----------------
I believe this code should be removed now; it is no longer reachable.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:1309-1311
+    if (!(getLangOpts().MicrosoftExt &&
+          TokenIsLikeStringLiteral(Tok, getLangOpts()) &&
+          TokenIsLikeStringLiteral(NextToken(), getLangOpts()))) {
----------------
I think this is good, but how about a comment to make the intent more clear?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1903
+  case tok::kw___PRETTY_FUNCTION__:
+    return PredefinedExpr::PrettyFunction;
+  }
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:2010-2013
+    OS << "R\"EFLPM("
+       << PredefinedExpr::ComputeName(getPredefinedExprKind(Tok.getKind()),
+                                      currentDecl)
+       << ")EFLPM\"";
----------------
"EFLPM"? I'm assuming this is an attempt to avoid an accidental clash with the computed name. Does this suffice to ensure a clash can't happen? If not, can we do better? Per http://eel.is/c++draft/lex.string#nt:r-char and http://eel.is/c++draft/lex.charset#1, we have a lot of flexibility regarding which characters to use.


================
Comment at: clang/test/Sema/ms_predefined_expr.cpp:115
+                                                                                                       // expected-warning{{string literal concatenation of predefined identifier 'L__FUNCSIG__' is a Microsoft extension}}
+}
+
----------------
How about testing some other encoding prefix combinations?
  u"" __FUNCTION // Ok; UTF-16 string literal
  u"" L__FUNCTION__ // ill-formed.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914