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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann requested changes to this revision.
tahonermann added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:118-120
+def ext_concat_predef_ms : ExtWarn<
+  "concatenation of predefined identifier '%0' is a Microsoft extension">,
+  InGroup<MicrosoftConcatPredefined>;
----------------
I think it makes since to be more explicit that the concatenation is with regard to string literals.


================
Comment at: clang/include/clang/Basic/TokenKinds.h:87-93
+/// Return true if this token is a predefined macro
+/// unexpandable by MSVC preprocessor.
+inline bool isUnexpandableMsMacro(TokenKind K) {
+  return K == tok::kw___FUNCTION__ || K == tok::kw___FUNCSIG__ ||
+         K == tok::kw_L__FUNCTION__ || K == tok::kw_L__FUNCSIG__ ||
+         K == tok::kw___FUNCDNAME__;
+}
----------------



================
Comment at: clang/include/clang/Parse/Parser.h:578-582
+  bool isTokenConcatenable() const {
+    return isTokenStringLiteral() ||
+           getLangOpts().MicrosoftExt &&
+               tok::isMSPredefinedMacro(Tok.getKind());
+  }
----------------
RIscRIpt wrote:
> cor3ntin wrote:
> > 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`
> Regarding `getLangOpts().MicrosoftExt`. If you are talking about it's presence in a function which name is meant to be used as a predicate, I agree. If you are talking about `class Parser`, then there're other places with references to `getLangOpts()`.
> 
> Without such function `ParseStringLiteralExpression` implementation would be too verbose.
> Let's decide what we can do after I address other comments.
> 
> Meanwhile, I renamed it to `isTokenLikeStringLiteral()`.
I suggest passing `getLangOpts()` to `isFunctionLocalPredefinedMacro` (`isUnexpandableMsMacro`) and letting it determine whether the token is or is not one of these special not-really-a-string-literal macro things. This will facilitate additional such macros controlled by different options while also colocating the option check with the related tokens.


================
Comment at: clang/include/clang/Sema/Sema.h:5681
 
+  std::vector<Token> ExpandUnexpandableMsMacros(ArrayRef<Token> Toks);
   ExprResult BuildPredefinedExpr(SourceLocation Loc,
----------------



================
Comment at: clang/lib/Parse/ParseExpr.cpp:1300-1307
+    if (!getLangOpts().MicrosoftExt || !tok::isUnexpandableMsMacro(SavedKind) ||
+        !tok::isUnexpandableMsMacro(NextToken().getKind()) &&
+            !tok::isStringLiteral(NextToken().getKind())) {
+      Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
+      ConsumeToken();
+      break;
+    }
----------------
Since the conditional code is only relevant for some of the tokens here, I would prefer to see the other token kinds (`kw___func__`, `kw___PRETTY_FUNCTION__`) handled separately to avoid the conditional fallthrough. That means duplicating the calls to `Actions.ActOnPredefinedExpr()` and `ConsumeToken()` between the blocks, but that bothers me less than having to understand the complicated condition.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3277-3280
+  // String concatenation.
+  // Note that keywords like __func__ and __FUNCTION__
+  // are not considered to be strings for concatenation purposes,
+  // unless Microsoft extensions are enabled.
----------------
I think `__func__` is not considered a string literal in Microsoft mode, so I think this comment needs some adjusting.


================
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");
----------------
What is the reason for requiring non-concatenated predefined function local macros to be handled by `ActOnPredefinedExpr()`?


================
Comment at: clang/lib/Sema/Sema.cpp:1494
 
+Decl *Sema::getCurScopeDecl() {
+  if (const BlockScopeInfo *BSI = getCurBlock())
----------------
`getCurScopeDecl` isn't a great name since this doesn't handle class or namespace scopes. How about `getCurLocalScopeDecl`? That name isn't quite right either since this can return a `TranslationUnitDecl`, but I think it matches the intent fairly well.

None of the callers need the actual `TranslationUnitDecl` that is returned. I think this function can return `nullptr` is the current scope isn't function local and callers can issue the relevant diagnostic in that case (thus avoiding the need to check that a translation unit decl was returned).


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1971-1974
+  // MSVC treats some of predefined identifiers (e.g. __FUNCTION__) as
+  // expandable macros defined as string literals, which may be concatenated.
+  // Expand them here (in Sema), because StringLiteralParser (in Lex)
+  // doesn't have access to AST.
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:2001-2004
+    OS << '"'
+       << Lexer::Stringify(PredefinedExpr::ComputeName(
+              getPredefinedExprKind(Tok.getKind()), currentDecl))
+       << '"';
----------------
A diagnostic is issued if the call to `getCurScopeDecl()` returned a translation unit declaration (at least in Microsoft mode). Does it make sense to pass a pointer to a `TranslationUnitDecl` here?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3694
+  Decl *currentDecl = getCurScopeDecl();
+  if (isa<TranslationUnitDecl>(currentDecl)) {
     Diag(Loc, diag::ext_predef_outside_function);
----------------
This can just be a `nullptr` check with my suggested change to `getCurScopeDecl()`.


================
Comment at: clang/test/Sema/ms_predefined_expr.cpp:62
+    static_assert(__FUNCTION__ ""_len == 14); // expected-warning{{concatenation of predefined identifier '__FUNCTION__' is a Microsoft extension}}
 }
----------------
If we don't already have such tests, please add tests that exercise these macros at global, namespace, and class scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914