[PATCH] D104299: Handle interactions between reserved identifier and user-defined suffixes

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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