RIscRIpt added inline comments.
================
Comment at: clang/include/clang/Parse/Parser.h:578-582
+ bool isTokenConcatenable() const {
+ return isTokenStringLiteral() ||
+ getLangOpts().MicrosoftExt &&
+ tok::isMSPredefinedMacro(Tok.getKind());
+ }
----------------
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()`.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1300
case tok::kw___PRETTY_FUNCTION__: // primary-expression: __P..Y_F..N__ [GNU]
- Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
- ConsumeToken();
+ Res = ParsePredefinedExpression(true);
break;
----------------
cor3ntin wrote:
> Can we instead, look at `NextToken()` and if next token is a StringLiteral or
> a MS predefined extension we fallthrough?
> That would avoid having duplicated logic in `ParsePredefinedExpression` and
> `ActOnStringLiteral` which would be a nice simplification
I thought of that, but I was afraid that playing with fallthrough wasn't
appreciated.
Thanks, fixed.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:3294-3298
StringToks.push_back(Tok);
- ConsumeStringToken();
- } while (isTokenStringLiteral());
+ if (isTokenStringLiteral())
+ ConsumeStringToken();
+ else
+ ConsumeToken();
----------------
cor3ntin wrote:
> I think I'd prefer `ConsumeAnyToken` with an assert that checks it's a
> stringLiteral or a predefined ms exppression
Done. But do we really need an assertion here? We have one in the function
preamble and strict condition in `while`.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:1955-1991
+ // MSVC treats some of predefined identifiers (e.g. __FUNCTION__) as
+ // expandable predefined macros defined as string literals,
+ // which may be concatenated. Expand them here (in Sema),
+ // because StringLiteralParser (in Lex) doesn't have access to AST.
+ std::vector<Token> ExpandedToks;
+ if (getLangOpts().MicrosoftExt) {
+ ExpandedToks = StringToks.vec();
----------------
cor3ntin wrote:
> Can we put that logic in a separate function?
Done. Tho, I couldn't make it `const` (for the same reason I couldn't make
`getCurScopeDecl() const`). And I wasn't sure about the interface:
```
std::vector<Token> ExpandMSPredefinedMacros(ArrayRef<Token> Toks);
```
vs
```
void ExpandMSPredefinedMacros(MutableArrayRef<Token> Toks);
```
================
Comment at: clang/lib/Sema/SemaExpr.cpp:1964
+ for (Token &Tok : ExpandedToks) {
+ if (!tok::isMSPredefinedMacro(Tok.getKind())) {
+ continue;
----------------
cor3ntin wrote:
> can you assert it's a string literal otherwise?
Done.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:1972-1987
+ SmallString<64> Str;
+ llvm::raw_svector_ostream OS(Str);
+ Token Exp;
+ Exp.startToken();
+ if (Tok.getKind() == tok::kw_L__FUNCTION__ ||
+ Tok.getKind() == tok::kw_L__FUNCSIG__) {
+ OS << 'L';
----------------
cor3ntin wrote:
> I think it might be easier to create a string_literal token directly here.
> I'm also not sure we need to use `Lexer::Stringify`
> I think it might be easier to create a string_literal token directly here.
What do you mean? Is there a function which creates Token object from
StringRef? Or is there a way to associate string literal value with a Token
without PP? I would like to simplify it, but I haven't found other ways of
achieving the same result.
> I'm also not sure we need to use Lexer::Stringify
Well, as far as I can see `StringLiteralParser` expands escape sequences. So, I
am just being too careful here.
If not using `Lexer::Stringify` do we want to assert that function name does
not contain neither `"` not `\` (which should not happen™)?
================
Comment at: clang/test/Sema/ms_predefined_expr.cpp:9
const char e[] = __PRETTY_FUNCTION__; // expected-warning{{initializing an
array from a '__PRETTY_FUNCTION__' predefined identifier is a Microsoft
extension}}
+ const wchar_t f[] = L__FUNCTION__; // expected-warning{{initializing an array
from a 'L__FUNCTION__' predefined identifier is a Microsoft extension}}
+ const wchar_t g[] = L__FUNCSIG__; // expected-warning{{initializing an array
from a 'L__FUNCSIG__' predefined identifier is a Microsoft extension}}
----------------
cor3ntin wrote:
> do we have any tests that look at the values of these things?
```
clang/test/Analysis/eval-predefined-exprs.cpp
clang/test/AST/Interp/literals.cpp
clang/test/SemaCXX/source_location.cpp
clang/test/SemaCXX/predefined-expr.cpp
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153914/new/
https://reviews.llvm.org/D153914
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
