This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Add warnings for CWG2521
ClosedPublic

Authored by rZhBoYao on Jun 10 2023, 2:13 PM.

Details

Summary
  1. Teach -Wuser-defined-literals to warn on using double underscores in literal suffix identifiers.
  2. Add -Wdeprecated-literal-operator to warn about the use of the first grammar production of literal-operator-id, which defaults to off for now.

Diff Detail

Event Timeline

rZhBoYao created this revision.Jun 10 2023, 2:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2023, 2:13 PM
rZhBoYao requested review of this revision.Jun 10 2023, 2:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2023, 2:13 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rZhBoYao added a reviewer: Restricted Project.Jun 10 2023, 2:18 PM

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?

Endill requested changes to this revision.Jun 10 2023, 11:51 PM
Endill added a subscriber: Endill.

Thank you for working on this!

Few questions:
I think this applies to all the language modes?

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.

This revision now requires changes to proceed.Jun 10 2023, 11:51 PM
rZhBoYao updated this revision to Diff 530292.Jun 11 2023, 3:40 AM

Addressed comments.
Thanks for letting me know how to structure tests for DRs.

Endill added inline comments.Jun 11 2023, 7:03 AM
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.

rZhBoYao updated this revision to Diff 530304.Jun 11 2023, 9:13 AM

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 🤔️.

rZhBoYao marked 2 inline comments as done.Jun 11 2023, 9:15 AM
aaron.ballman added inline comments.
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!

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.

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 @

rZhBoYao added inline comments.Jun 13 2023, 9:58 AM
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,
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!

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

rZhBoYao updated this revision to Diff 532223.Jun 16 2023, 10:49 AM
rZhBoYao edited the summary of this revision. (Show Details)

Depends on D153156

PS: I'm going on a vacation for 2 weeks. See you guys in July :)

shafik added a subscriber: shafik.Jun 22 2023, 8:40 PM

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.

rZhBoYao updated this revision to Diff 538678.Jul 10 2023, 9:06 AM
rZhBoYao edited the summary of this revision. (Show Details)

Defaults to on for C++23 only. Enable for all the language modes in another patch.

rZhBoYao marked 6 inline comments as done.EditedJul 10 2023, 9:43 AM

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

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.

Endill accepted this revision as: Endill.Jul 11 2023, 6:14 AM

DR testing side looks good, but you should wait for more approvals.
Thank you for addressing all the issues there!

Endill accepted this revision.Jul 11 2023, 6:15 AM
This revision is now accepted and ready to land.Jul 11 2023, 6:15 AM

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?

rZhBoYao updated this revision to Diff 540072.EditedJul 13 2023, 9:30 AM
rZhBoYao marked 6 inline comments as done.
rZhBoYao edited the summary of this revision. (Show Details)

-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

aaron.ballman accepted this revision.Jul 13 2023, 10:37 AM

LGTM assuming precommit CI doesn't find any surprises.

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

This revision was automatically updated to reflect the committed changes.