This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Implement fix for DR2628
ClosedPublic

Authored by royjacobson on Sep 18 2022, 1:12 PM.

Details

Summary

Implement suggested fix for DR2628. Couldn't update the DR docs because there hasn't been a DR index since it was filed, but the tests still run in CI.

Note: I only transfer the constructor constraints, not the struct constraints. I think that's OK because the struct constraints are the same
for all constructors so they don't affect the overload resolution, and if they deduce to something that doesn't pass the constraints
we catch it anyway. So (hopefully) that should be more efficient without sacrificing correctness.

Closes:
https://github.com/llvm/llvm-project/issues/57646
https://github.com/llvm/llvm-project/issues/43829

Diff Detail

Event Timeline

royjacobson created this revision.Sep 18 2022, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2022, 1:12 PM
royjacobson requested review of this revision.Sep 18 2022, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2022, 1:12 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
royjacobson edited the summary of this revision. (Show Details)Sep 18 2022, 1:15 PM
royjacobson added reviewers: erichkeane, shafik, ychen.
erichkeane accepted this revision.Sep 19 2022, 7:08 AM

This seems right to me, thanks!

I'm currently pushing to try to make our diagnostics 'check' lines more readable... it doesn't gain a ton in this case, but more to try to push for consistency.

clang/test/CXX/drs/dr26xx.cpp
7
12
This revision is now accepted and ready to land.Sep 19 2022, 7:08 AM
royjacobson edited the summary of this revision. (Show Details)

Add release note, slightly nicer test

clang/test/CXX/drs/dr26xx.cpp
12

That does look much nicer, thanks! I qualified it with DR2628 because the DR tests can get pretty large.

rebase on main

This revision was landed with ongoing or failed builds.Sep 19 2022, 2:07 PM
This revision was automatically updated to reflect the committed changes.