[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3586-3591
+  // MSVC treats all predefined expressions as string literals rather than char
+  // arrays.
+  if (LangOpts.MicrosoftExt)
+    return SL;
+
   return PredefinedExpr::Create(Context, Loc, ResTy, IK, SL);
----------------
aeubanks wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > aeubanks wrote:
> > > > aaron.ballman wrote:
> > > > > This is incorrect -- these are still predefined expressions even if they're a string literal. Otherwise, we lose AST fidelity and things like the `predefinedExpr()` AST matcher don't work (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/ASTMatchers/ASTMatchers.h#L2697), and `-ast-print` will print the wrong thing (https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/StmtPrinter.cpp#L1248).
> > > > how would you structure this? an almost identical `PredefinedExpr` class that also subclasses `StringLiteral`? or special case all the places that check for `StringLiteral` to also check for `PredefinedExpr` (the case I was mostly looking at was initializing a char array with predefined expressions but presumably there are more places that matter)
> > > We can't inherit from `StringLiteral` (it's a `final` class because it has `TrailingObjects`), so that might be the only viable approach. However, there's a fair number of places in the compiler where we care if something is or isn't a `StringLiteral`, so I can see why we'd want to avoid that if possible.
> > > 
> > > Oye, but it seems that even MSVC is not consistent with whether the predefined expression is a string literal or not: https://godbolt.org/z/jzY8cz79d
> > I think these should be plain old string literals, but for the purpose or AST matchers and such, maybe we could have a "predefined macro" bit, or even an additional object that stores the name.
> > Oye, but it seems that even MSVC is not consistent with whether the predefined expression is a string literal or not: https://godbolt.org/z/jzY8cz79d
> 
> the other predefined expressions work, that looks like a bug on MSVC's part
> 
> > I think these should be plain old string literals, but for the purpose or AST matchers and such, maybe we could have a "predefined macro" bit, or even an additional object that stores the name.
> 
> what do you mean by "an additional object that stores the name"?
> 
> I started trying to add a bit to `StringLiteral` that signifies if it's an MSVC predefined expression. being new to ast matchers, I'm having trouble figuring out how to make a matcher that matches both `PredefinedExpr` and some `StringLiteral`s but also behaves like a `VariadicDynCastAllOfMatcher`.
> 
> something I noticed in one of the tests [1] is that there's a check
> ```
> predefinedExpr(hasType(asString("const char[4]")),
>                                      has(stringLiteral()))
> ```
> thinking about the `has(stringLiteral())`, if somebody wants to get the literal for a predefined expr, presumably right now they're looking for a string literal child, but if we directly match a string literal then that would break? WDYT?
> 
> [1] https://github.com/llvm/llvm-project/blob/75ca15fcbb2e1b3671e41f971a000c6d59f5e5ae/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp#L1064
I wonder if we could keep the representation as-is, but add an "is transparent" bit on `PredefinedExpr` that's set for string literal predefined expressions under `-fms-extensions`, and make `IgnoreParens` step over transparent `PredefinedExpr`s. That seems like it'd exactly match the MS behavior, which seems to allow (eg) `__func__` to appear anywhere a //parenthesized// string literal can appear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764