Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
5730–5732 | 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. | |
5736–5737 | 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. | |
5745–5746 | Also adds back the isDependent flag. Any reason to have removed it? | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
8664–8668 | 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. |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
5736–5737 |
Thanks. Great to know. | |
5745–5746 | I was thinking the replacement type is null, so there is no need to depend on instantiation? | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
8664–8668 |
RT->getContainedAutoType() will be called only when RT is AutoType? |
clang/lib/Sema/SemaDeclCXX.cpp | ||
---|---|---|
8664–8668 | Oh right, I see it now, there is a lot going on in that condition :) |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
5745–5746 |
| |
5745–5746 | 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. |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
5745–5746 | 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. |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
5745–5746 | Yeah, I'm not confident to remove it right now. Already put it back. |
As a nit, I'd probably just call this TC or TypeConstr.