[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/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__;
+}
----------------
RIscRIpt wrote:
> tahonermann wrote:
> > 
> Thanks, I like the name. But shouldn't we reflect that we are referring to only Microsoft (unexpandable) macros? How about `isFunctionLocalPredefinedMsMacro`?
I don't think the Microsoft association is meaningful. Sure, some of the predefined macros are only treated differently when compiling in Microsoft mode, but some of those macros aren't Microsoft specific. Also, there might be macros provided by other implementations that we'll want to emulate some day.


================
Comment at: clang/include/clang/Sema/Sema.h:5681
 
+  std::vector<Token> ExpandUnexpandableMsMacros(ArrayRef<Token> Toks);
   ExprResult BuildPredefinedExpr(SourceLocation Loc,
----------------
RIscRIpt wrote:
> tahonermann wrote:
> > 
> Renamed to `ExpandFunctionLocalPredefinedMsMacros`. If you think my addition of `Ms` is redundant, let me know.
I don't think the `Ms` is redundant, but I do think it is unnecessary and slightly confusing (`__FUNCTION__`) is not a Microsoft macro.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:1310-1311
+  case tok::kw_L__FUNCSIG__:  // primary-expression: L__FUNCSIG__ [MS]
+    if (!getLangOpts().MicrosoftExt ||
+        !TokenIsLikeStringLiteral(NextToken(), getLangOpts())) {
+      Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
----------------
`TokenIsLikeStringLiteral()` checks `MicrosoftExt`, so the check here is redundant. This is an example of why I would like to see the `MicrosoftExt` checking pushed down to `isFunctionLocalPredefinedMsMacro()`; that really seems where that checking belongs to me.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3298-3299
+  assert(
+      (StringToks.size() != 1 ||
+       !tok::isFunctionLocalPredefinedMsMacro(StringToks[0].getKind())) &&
+      "single predefined identifiers shall be handled by ActOnPredefinedExpr");
----------------
Is there a missing check for `MicrosoftExt` here? This is another example of why I would prefer to see those checks pushed down to `isFunctionLocalPredefinedMsMacro()`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1979-1980
+  Decl *currentDecl = getCurLocalScopeDecl();
+  if (!currentDecl)
+    currentDecl = Context.getTranslationUnitDecl();
+  std::vector<Token> ExpandedToks;
----------------
This could use a comment since it isn't obvious why the translation unit declaration is used when not in a local scope declaration of some kind.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:2001-2004
+    OS << '"'
+       << Lexer::Stringify(PredefinedExpr::ComputeName(
+              getPredefinedExprKind(Tok.getKind()), currentDecl))
+       << '"';
----------------
RIscRIpt wrote:
> tahonermann wrote:
> > 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?
> Shortly, yes, kind of. `ComputeName` can handle `TranslationUnitDecl` in which case it returns an empty string. This way we implicitly (without additional code) create empty string-literal Tokens which can be handled in `StringLiteralParser`. And we cannot pass non-string-literal tokens into `StringLiteralParser`.
Ah, ok. And I see it even differentiates what name is produced for `__PRETTY_FUNCTION__`. That leaves me wondering what the right behavior should be at class and namespace scope, but maybe I'm better off not asking questions I don't want to know the answer to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914