aaron.ballman 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< ---------------- 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. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9268-9271 def warn_user_literal_reserved : Warning< - "user-defined literal suffixes not starting with '_' are reserved" + "user-defined literal suffixes containing '__' or not starting with '_' are reserved" "%select{; no literal will invoke this operator|}0">, InGroup<UserDefinedLiterals>; ---------------- Hmmm, you've moved things in the right direction, but I think this should also live under the `-Wreserved-identifier` group given that it's about reserved identifiers. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16448 = FnDecl->getDeclName().getCXXLiteralIdentifier()->getName(); - if (LiteralName[0] != '_' && + if ((LiteralName[0] != '_' || LiteralName.contains("__")) && !getSourceManager().isInSystemHeader(FnDecl->getLocation())) { ---------------- I think we should use `IdentifierInfo::isReserved()` to determine why it's reserved. ================ 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()); } ---------------- Hmmm this means we're issuing two diagnostics -- missing an `else` here? ================ 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}} ---------------- So we give an error if there's not a space, but then give a deprecation warning when there is a space? 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
