rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Thanks, looks good. Just some minor comments.
================
Comment at: clang/include/clang/Sema/Sema.h:4132
+ bool checkLiteralOperatorId(const CXXScopeSpec &SS, const UnqualifiedId &Id,
+ bool isUDSuffix);
LiteralOperatorLookupResult
----------------
Style nit: please start variables with uppercase letters.
================
Comment at: clang/lib/AST/Decl.cpp:1084-1086
if (!II)
if (const auto *FD = dyn_cast<FunctionDecl>(this))
II = FD->getLiteralIdentifier();
----------------
Can you now delete this along with the added code below?
================
Comment at: clang/lib/AST/Decl.cpp:1091
+ // already checked at lexing time
+ if (getDeclName().getCXXLiteralIdentifier())
----------------
Style nit: comments should start with a capital letter and end in a period.
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:489-499
+ if (!isUDSuffix) {
+ // [over.literal] p8
+ IdentifierInfo *II = Name.Identifier;
+ auto Status = II->isReserved(PP.getLangOpts());
+ auto Loc = Name.getBeginLoc();
+ if (Status != ReservedIdentifierStatus::NotReserved &&
+ !PP.getSourceManager().isInSystemHeader(Loc)) {
----------------
Hmm, interesting. This differs from the behavior of regular identifiers in that
it will warn on both declarations and uses of reserved literal operator names.
But I think that's actually a desirable difference! For example:
```
int _Foo(); // should warn here (if not in system header)
int k1 = _Foo(); // no need to warn here, and we shouldn't if the previous line
is in a system header
int operator""_Foo();
int k2 = operator"" _Foo(); // should warn here, regardless of whether the
previous line is in a system header
```
Given that each appearance can make a different choice in this regard, and that
the problem can be fixed locally by removing the space, warning on each
appearance seems like the right approach.
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:490
+ if (!isUDSuffix) {
+ // [over.literal] p8
+ IdentifierInfo *II = Name.Identifier;
----------------
It's useful to also include a brief quotation of the relevant text and a
standard version, since paragraphs get moved around and renumbered over time.
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:497
+ Diag(Loc, diag::warn_reserved_extern_symbol)
+ << II << static_cast<int>(Status);
+ }
----------------
Can we produce a `FixItHint` to remove the space? (That might require the
parser to pass through a little more information, such as the location for the
end of the final string literal token, so we can reconstruct the space range
here.)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104299/new/
https://reviews.llvm.org/D104299
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
