This is an archive of the discontinued LLVM Phabricator instance.

[Modules] Add ODR Check for concepts
ClosedPublic

Authored by ChuanqiXu on Jul 4 2022, 8:41 PM.

Details

Summary

Closing https://github.com/llvm/llvm-project/issues/56310

Previously we don't check the contents of concept so it might merge inconsistent results.

Important Note: this patch might break existing code but the behavior might be right.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Jul 4 2022, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2022, 8:41 PM
ChuanqiXu requested review of this revision.Jul 4 2022, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2022, 8:41 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ChuanqiXu updated this revision to Diff 442179.Jul 4 2022, 11:01 PM

Undo the format test. It breaks the testing.

erichkeane added inline comments.Jul 5 2022, 6:26 AM
clang/lib/AST/ASTContext.cpp
6532

This isn't necessarily valid here, you did a dyn_cast above.

6533
ChuanqiXu updated this revision to Diff 442425.Jul 5 2022, 7:13 PM

Address comments.

clang/lib/AST/ASTContext.cpp
6532

Oh, my bad. I should use cast. We could find the style at line 6525-6526 and line 6519-6520.

ChuanqiXu marked an inline comment as done.Jul 7 2022, 11:38 PM
ChuanqiXu added inline comments.
clang/lib/AST/ASTContext.cpp
6533

The assertion here should be fine since the '=' part is guaranteed by std: https://eel.is/c++draft/temp.concept#1 and clang would reject it too: https://godbolt.org/z/Tfe493xK7

erichkeane accepted this revision.Jul 12 2022, 7:03 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 12 2022, 7:03 AM

Should have a release note on commit!

Thanks for reviewing and noting!

ChuanqiXu updated this revision to Diff 443957.Jul 12 2022, 8:23 AM

Add ReleaseNotes.

erichkeane added inline comments.Jul 12 2022, 8:25 AM
clang/docs/ReleaseNotes.rst
186 ↗(On Diff #443957)
187 ↗(On Diff #443957)
ChuanqiXu updated this revision to Diff 443958.Jul 12 2022, 8:29 AM

Address comments.

ChuanqiXu marked 2 inline comments as done.Jul 12 2022, 8:31 AM
ChuanqiXu added inline comments.
clang/docs/ReleaseNotes.rst
187 ↗(On Diff #443957)

Although as a non-native speaker, the current wording looks like the new clang is going to break existing code and what I want to say it is only possible.

I followed your suggestion since I feel it might not be meaningful to argue English with a native speaker : )

erichkeane added inline comments.Jul 12 2022, 8:37 AM
clang/docs/ReleaseNotes.rst
187 ↗(On Diff #443957)

Hmm, you're right about that being a subtlety that I didn't pick up.

We could consider changing 'expected to' to 'likely to', or replace 'is expected to' could be 'may' (or even 'may possibly') depending on your confidence :)

ChuanqiXu updated this revision to Diff 443961.Jul 12 2022, 8:43 AM
ChuanqiXu marked an inline comment as done.

Address comments.

ChuanqiXu marked an inline comment as done.Jul 12 2022, 8:43 AM
This revision was landed with ongoing or failed builds.Jul 12 2022, 8:46 AM
This revision was automatically updated to reflect the committed changes.