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@+2 {{invalid suffix on literal; C++11 requires a space between literal and identifier}} + // expected-warning@+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@+2 {{identifier '_π___' preceded by space(s) in the literal operator declaration is deprecated}} +// expected-warning@+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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
