tahonermann requested changes to this revision.
tahonermann added a comment.
This revision now requires changes to proceed.
I think this is looking good. The only big thing I noticed is some code that
looks like it should have been removed with the last set of changes. Otherwise,
just some minor concerns and suggestions.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1299-1301
+ Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
+ ConsumeToken();
+ break;
----------------
I believe this code should be removed now; it is no longer reachable.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1309-1311
+ if (!(getLangOpts().MicrosoftExt &&
+ TokenIsLikeStringLiteral(Tok, getLangOpts()) &&
+ TokenIsLikeStringLiteral(NextToken(), getLangOpts()))) {
----------------
I think this is good, but how about a comment to make the intent more clear?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:1903
+ case tok::kw___PRETTY_FUNCTION__:
+ return PredefinedExpr::PrettyFunction;
+ }
----------------
================
Comment at: clang/lib/Sema/SemaExpr.cpp:2010-2013
+ OS << "R\"EFLPM("
+ << PredefinedExpr::ComputeName(getPredefinedExprKind(Tok.getKind()),
+ currentDecl)
+ << ")EFLPM\"";
----------------
"EFLPM"? I'm assuming this is an attempt to avoid an accidental clash with the
computed name. Does this suffice to ensure a clash can't happen? If not, can we
do better? Per http://eel.is/c++draft/lex.string#nt:r-char and
http://eel.is/c++draft/lex.charset#1, we have a lot of flexibility regarding
which characters to use.
================
Comment at: clang/test/Sema/ms_predefined_expr.cpp:115
+
// expected-warning{{string literal concatenation of
predefined identifier 'L__FUNCSIG__' is a Microsoft extension}}
+}
+
----------------
How about testing some other encoding prefix combinations?
u"" __FUNCTION // Ok; UTF-16 string literal
u"" L__FUNCTION__ // ill-formed.
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
