- Teach -Wuser-defined-literals to warn on using double underscores in literal suffix identifiers.
- Add -Wdeprecated-literal-operator to warn about the use of the first grammar production of literal-operator-id, which defaults to off for now.
Details
- Reviewers
serge-sans-paille Endill aaron.ballman - Group Reviewers
Restricted Project - Commits
- rG5ce5e983f82c: [Clang] Add warnings for CWG2521
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Few questions:
I think this applies to all the language modes?
Somehow, https://wg21.link/CWG2521 is not redirecting to https://cplusplus.github.io/CWG/issues/2521.html. Is this normal?
Thank you for working on this!
Yes, we rarely limit the scope of DRs, and I'm looking forward to get rid of exceptions from this approach (grep "since" in cxx_dr_status.html).
Somehow, https://wg21.link/CWG2521 is not redirecting to https://cplusplus.github.io/CWG/issues/2521.html. Is this normal?
It happens, especially with recently updated DRs. I see it in CWG index, so you can ignore broken wg21.link.
clang/www/cxx_dr_status.html | ||
---|---|---|
14936 | This file shouldn't be edited manually. You should write a test in clang/test/CXX/drs/dr25xx.cpp, and then run clang/www/make_cxx_dr_status script that updates cxx_dr_status.html. |
clang/test/CXX/drs/dr25xx.cpp | ||
---|---|---|
2 | We avoid passing compiler options here, because it affects all the tests in the file. If you need to enable or disable a specific diagnostic, better wrap your test with #pragma clang diagnostic push #pragma clang diagnostic warning "-Wdeprecated-literal-operation" // or "ignored" instead of "warning" // ... #pragma clang diagnostic pop That said, files containing newer DRs are not too well maintained at the moment (but I'll get to them at some point). This file in particular lacks RUN lines for all missing language modes, and -pedantic-errors. I guess the latter enables diagnostics that are needed for your test. You can use dr5xx.cpp as an example. |
Overhaul for dr25xx.cpp.
For each test case, tried to support as many language modes as possible.
Not sure what those -triple x86_64-unknown-unknown are for? I leave them there nonetheless.
-Wdeprecated-literal-operator is under -Wdeprecated which is not under -Wpedantic so is not triggered by -pedantic-errors. Does this need change? I imagine it would be pretty disruptive. On the other hand, pedantic users might care about deprecation 🤔️.
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
411 | 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. | |
9266–9269 | 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. | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
16450–16452 | I think we should use IdentifierInfo::isReserved() to determine why it's reserved. | |
clang/lib/Sema/SemaExprCXX.cpp | ||
512–517 | Hmmm this means we're issuing two diagnostics -- missing an else here? | |
clang/test/CXX/drs/dr17xx.cpp | ||
129 ↗ | (On Diff #530304) | So we give an error if there's not a space, but then give a deprecation warning when there is a space? |
Thank you for taking care of other tests under dr25xx.cpp!
They are usually used for codegen tests, so no need to specify the triple if you don't need to.
-Wdeprecated-literal-operator is under -Wdeprecated which is not under -Wpedantic so is not triggered by -pedantic-errors. Does this need change? I imagine it would be pretty disruptive. On the other hand, pedantic users might care about deprecation 🤔️.
I don't think we need to change status quo. In general I don't expect diagnostics grouping to care about DR tests, and I don't think it should.
clang/test/CXX/drs/dr25xx.cpp | ||
---|---|---|
71 | 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 @ |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
411 | I'm thinking the same after seeing 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. | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
16450–16452 | Totally. But I think we need another function, maybe isReservedLiteralSuffixId, since the rule for literal suffix identifiers is very different from that for "identifiers appearing as a token or preprocessing-token". | |
clang/lib/Sema/SemaExprCXX.cpp | ||
512–517 | 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, Oh wait, issuing two fixit hint seems to be very bad! | |
clang/test/CXX/drs/dr17xx.cpp | ||
129 ↗ | (On Diff #530304) | I found it very confusing when editing the next line. After some research, I think that came from C++11 over.literal. But the rule should have long been superseded by CWG1473 right? Since DRs are applied retroactively. Additionally, GCC doesn't warn on this. Should we retire this error? |
clang/test/CXX/drs/dr25xx.cpp | ||
71 | Will do |
I am wondering why we don't fold this into -Wreserved-identifier
clang/include/clang/Basic/IdentifierTable.h | ||
---|---|---|
56 | I would think starting with a double underscore would also not be allowed, at least that is my reading. |
To quota https://eel.is/c++draft/over.literal#1.sentence-2
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, reserved-identifier-wise.
Thus, I don't think folding -Wuser-defined-literals into -Wreserved-identifier is a good idea.
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
411 | Default on and diagnose for C++23 only. Enable for all the lang modes in https://reviews.llvm.org/D153156 | |
clang/include/clang/Basic/IdentifierTable.h | ||
56 | That is indeed the case. |
DR testing side looks good, but you should wait for more approvals.
Thank you for addressing all the issues there!
This breaks libc++: https://buildkite.com/llvm-project/clang-ci/builds/26#01894b7d-8d6f-4893-ab14-1147229390dc
Doesn't mean don't land it, but please try to address the issues in this patch to keep everyone green.
Mostly just minor nits from me, otherwise this is looking close to good! If you're able to handle the libc++ failures as part of this patch as well, that'd be great (but isn't required for landing the changes).
clang/docs/ReleaseNotes.rst | ||
---|---|---|
145–148 | ||
157–158 | ||
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
410–411 | ||
clang/lib/Sema/SemaDeclCXX.cpp | ||
16450–16453 | Please spell out the types instead of using auto. | |
clang/lib/Sema/SemaExprCXX.cpp | ||
514 | Can you pass in II here instead of II->getName() or does that give bad behavior? |
-Wdeprecated-literal-operator will be on by default in https://reviews.llvm.org/D153156
This one is now blocked by the libc++ fix: https://reviews.llvm.org/D155200
Hmm… I don’t see why check-format keeps failing. I git clang-format thrice before uploading.
https://buildkite.com/llvm-project/premerge-checks/builds/164452