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

Mon Jul 10 09:43:56 PDT 2023

rZhBoYao marked 6 inline comments as done.
rZhBoYao added a comment.

In D152632#4443284 <https://reviews.llvm.org/D152632#4443284>, @shafik wrote:

> I am wondering why we don't fold this into `-Wreserved-identifier`

The "ud-suffix" of the user-defined-string-literal or the identifier in a literal-operator-id is called a literal suffix identifier.
However "//ud-suffix//" and "identifiers appearing as a //token// or //preprocessing-token//" are treated differently in the standard.
Thus, I don't think folding -Wuser-defined-literals into -Wreserved-identifier is a good idea.

================
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<
----------------
rZhBoYao wrote:
> 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.
Default on and diagnose for C++23 only. Enable for all the lang modes in https://reviews.llvm.org/D153156

================
Comment at: clang/include/clang/Basic/IdentifierTable.h:56
+  NotStartsWithUnderscore,
+  ContainsDoubleUnderscore,
+};
----------------
shafik wrote:
> I would think starting with a double underscore would also not be allowed, at least that is my reading.
That is indeed the case.

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

https://reviews.llvm.org/D152632