[PATCH] D152632: [Clang] Add warnings for CWG2521

[PoYao Chang via Phabricator via cfe-commits]

rZhBoYao added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:411
+  "identifier '%0' preceded by space(s) in the literal operator declaration "
+  "is deprecated">, InGroup<DeprecatedLiteralOperator>, DefaultIgnore;
 def warn_reserved_module_name : Warning<
----------------
aaron.ballman wrote:
> Oye, I'm of two minds about `DefaultIgnore`.
> 
> On the one hand, this is going to fire *a lot*: https://sourcegraph.com/search?q=context:global+operator%5B%5B:space:%5D%5D*%5C%22%5C%22%5B%5B:space:%5D%5D%5BA-Za-z0-9_%5D%2B&patternType=regexp&case=yes&sm=1&groupBy=repo
> 
> On the other hand, if we don't warn about it being deprecated, users won't know about it, which makes it harder to do anything about it the longer we silently allow it. (We have plenty of experience with this particular flavor of pain.)
> 
> I think we should warn about this by default. It's under its own warning group, so users who want to ignore the warning and live dangerously can do so. And it will be silenced in system headers automatically, so issuing the diagnostic should generally be actionable for users.
I'm thinking the same after seeing [[ https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2863r0.html#D.9 | P2863R0 § 6.9 ]]

However, a lot of tests are written in the deprecated way. I think a patch removing all the whitespaces only preceding this one is needed, to not clutter up this one.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16448
     = FnDecl->getDeclName().getCXXLiteralIdentifier()->getName();
-  if (LiteralName[0] != '_' &&
+  if ((LiteralName[0] != '_' || LiteralName.contains("__")) &&
       !getSourceManager().isInSystemHeader(FnDecl->getLocation())) {
----------------
aaron.ballman wrote:
> I think we should use `IdentifierInfo::isReserved()` to determine why it's reserved.
Totally. But I think we need another function, maybe isReservedLiteralSuffixId, since the rule for `literal suffix identifier`s is very different from that for "identifiers appearing as a token or preprocessing-token".


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:512-517
+      Diag(Loc, diag::warn_deprecated_literal_operator_id)
+          << II->getName()
           << FixItHint::CreateReplacement(
                  Name.getSourceRange(),
                  (StringRef("operator\"\"") + II->getName()).str());
     }
----------------
aaron.ballman wrote:
> Hmmm this means we're issuing two diagnostics -- missing an `else` here?
Does it have to be one or the other?

For example:
```
long double operator""      _KM(long double); // only warn reserved identifier
```
After replacing the identifier with km and recompile,
now there would be a new warning that was not previously shown,
which may be quite frustrating.

-------------
Oh wait, issuing two fixit hint seems to be very bad!





================
Comment at: clang/test/CXX/drs/dr17xx.cpp:129
   float operator ""_E(const char *);
   // expected-error at +2 {{invalid suffix on literal; C++11 requires a space between literal and identifier}}
+  // expected-warning at +1 {{user-defined literal suffixes containing '__' or not starting with '_' are reserved; no literal will invoke this operator}}
----------------
aaron.ballman wrote:
> So we give an error if there's not a space, but then give a deprecation warning when there is a space?
I found it very confusing when editing the next line. After some research, I think that came from [[ https://timsong-cpp.github.io/cppwp/n3337/over.literal | C++11 over.literal ]]. But the rule should have long been superseded by [[ http://wg21.link/cwg1473 |CWG1473 ]] right? Since DRs are applied retroactively.

Additionally, GCC doesn't warn on this. Should we retire this error?


================
Comment at: clang/test/CXX/drs/dr25xx.cpp:71
+// expected-warning at +2 {{identifier '_π___' preceded by space(s) in the literal operator declaration is deprecated}}
+// expected-warning at +1 {{user-defined literal suffixes containing '__' or not starting with '_' are reserved}}
+long double operator""      _\u03C0___(long double);
----------------
Endill wrote:
> Is it possible to put expected directive after the code, like we do in majority of existing tests? This means using only negative line offsets after `@`
Will do


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152632/new/

https://reviews.llvm.org/D152632