[PATCH] D104299: Handle interactions between reserved identifier and user-defined suffixes
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 17 18:04:55 PDT 2021
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
More information about the cfe-commits
mailing list