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

[Richard Dzenis via Phabricator via cfe-commits]

RIscRIpt added inline comments.


================
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);
----------------
tahonermann wrote:
> `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.
Let's consider this code:
```
    if (!TokenIsLikeStringLiteral(NextToken(), getLangOpts())) {
      Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
      ConsumeToken();
      break;
    }
```

The condition can be expanded as follows: `!(isStringLiteral || (MsExt && NeededToken))`. Let's consider `MsExt` is `false`, then we have: `!isStringLiteral`. So, if next token is StringLiteral we will try to concatenate it even without MsExt, which is invalid. Thus this seemingly redundant check is needed.


================
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");
----------------
tahonermann wrote:
> 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()`.
My intention for this `assert` is to ensure that we keep treating single predefined identifiers as `PredefinedExpr` (when we don't need to concat them) to preserve AST. I've adjusted the check to make it more clear.


================
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;
+    }
----------------
RIscRIpt wrote:
> tahonermann wrote:
> > 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.
> That's a valid point. And by handling two tokens separately allows simplification of the condition. Adjusted code.
Having `TokenIsLikeStringLiteral` I think we can return to single case. I believe 3 values in condition is not difficult.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914