Page MenuHomePhabricator

[Clang] make canonical AutoType constraints-free
ClosedPublic

Authored by ychen on Oct 3 2022, 10:20 AM.

Details

Summary

As @mizvekov suggested in D134772. This works great for D128750 when
dealing with AutoType's.

Diff Detail

Event Timeline

ychen created this revision.Oct 3 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2022, 10:20 AM
ychen requested review of this revision.Oct 3 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2022, 10:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
erichkeane accepted this revision.Oct 3 2022, 10:24 AM

This looks right enough to me with the 1 nit, but let Matheus take a look before committing.

clang/lib/AST/ASTContext.cpp
699

As a nit, I'd probably just call this TC or TypeConstr.

This revision is now accepted and ready to land.Oct 3 2022, 10:24 AM
ychen updated this revision to Diff 464731.Oct 3 2022, 10:30 AM
ychen marked an inline comment as done.Oct 3 2022, 10:30 AM

This looks right enough to me with the 1 nit, but let Matheus take a look before committing.

Thanks for the quick review.

mizvekov added inline comments.Oct 3 2022, 4:36 PM
clang/lib/AST/ASTContext.cpp
5757–5758

Pre-existing problem: This was not good to begin with, since we would not capture the correct declaration used, but now you can just remove it, no further changes required.

5761–5762

FYI (not asking you to fix this on this patch) there is a pre-existing bug here where we are not profiling the node on the isPack flag.

5768–5769

Also adds back the isDependent flag. Any reason to have removed it?

clang/lib/Sema/SemaDeclCXX.cpp
8669–8674

I think this might crash if the three-way comparison is defaulted with a deduced template specialization.

Something like:

template <class> struct A {};

struct P {
  friend A operator<=>(const P&, const P&) = default;
};

The problem is that RT->getContainedDeducedType() will return non-null, because DTSTs are also deduced types, but RT->getContainedAutoType() will return null, because DTSTs are not AutoTypes.

ychen added inline comments.Oct 3 2022, 5:06 PM
clang/lib/AST/ASTContext.cpp
5761–5762

FYI (not asking you to fix this on this patch) there is a pre-existing bug here where we are not profiling the node on the isPack flag.

Thanks. Great to know.

5768–5769

I was thinking the replacement type is null, so there is no need to depend on instantiation?

clang/lib/Sema/SemaDeclCXX.cpp
8669–8674

I think this might crash if the three-way comparison is defaulted with a deduced template specialization.
The problem is that RT->getContainedDeducedType() will return non-null, because DTSTs are also deduced types, but RT->getContainedAutoType() will return null, because DTSTs are not AutoTypes.

RT->getContainedAutoType() will be called only when RT is AutoType?

ychen updated this revision to Diff 464858.Oct 3 2022, 5:19 PM
  • remove dead code
ychen marked an inline comment as done.Oct 3 2022, 5:19 PM
mizvekov added inline comments.Oct 3 2022, 5:31 PM
clang/lib/Sema/SemaDeclCXX.cpp
8669–8674

Oh right, I see it now, there is a lot going on in that condition :)

ychen added inline comments.Oct 3 2022, 5:39 PM
clang/lib/AST/ASTContext.cpp
5768–5769

I was thinking the replacement type is null, so there is no need to depend on instantiation?

5768–5769

Sorry I was not precise. I meant the constrained non-canonical AutoType would always be used for template instantiation. But I think it could happen that the user may query dependencies of the canonical AutoType of a constrained AutoType, maybe it just doesn't show up in the codebase yet. I'll put it back.

ychen updated this revision to Diff 464861.Oct 3 2022, 5:45 PM
  • add back IsDependent parameter.
mizvekov added inline comments.Oct 3 2022, 5:50 PM
clang/lib/AST/ASTContext.cpp
5768–5769

FYI my memory is fuzzy right now, but we do have the distinction between undeduced but non-dependent, and undeduced but dependent. It's suspicious that no tests fail though. It could be either it doesn't matter for the canonical type, or that the test would be a bit convoluted.

ychen added inline comments.Oct 3 2022, 5:54 PM
clang/lib/AST/ASTContext.cpp
5768–5769

Yeah, I'm not confident to remove it right now. Already put it back.

mizvekov accepted this revision.Oct 3 2022, 6:03 PM
mizvekov added inline comments.
clang/lib/AST/ASTContext.cpp
5767–5768

Last minor nit, but I think this looks more readable if you invert the isNull check and do the TypeConstraintConcept on the else of that, less nesting.

6333–6336

isSameConstraintExpr already handles that.

ychen updated this revision to Diff 464865.Oct 3 2022, 6:11 PM
ychen marked an inline comment as done.
  • Address comments.
ychen marked an inline comment as done.Oct 3 2022, 6:12 PM

@mizvekov Thanks for the review!

clang/lib/AST/ASTContext.cpp
5767–5768

Yep. That's better.

6333–6336

Ack.

This revision was automatically updated to reflect the committed changes.
ychen marked an inline comment as done.