aeubanks 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); ---------------- aaron.ballman wrote: > rsmith wrote: > > 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. > That seems like a good approach to me. We've got spare bits in > `PredefinedExprBitfields`, so this won't impact AST size, which is nice. much much cleaner, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146764/new/ https://reviews.llvm.org/D146764 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
