mizvekov added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:8285
+ TemplateArgument &SugaredConverted,
+ TemplateArgument &CanonicalConverted,
+ CheckTemplateArgumentKind CTAK);
----------------
erichkeane wrote:
> I find myself wondering if we should just be passing around the 'Sugared'
> version, and canonicalizing when we need it? Is there a good reason not to?
I think given the size of the patch, splitting things this way makes it way
easier to not pass the wrong list somewhere, and then spend a long time looking
for the problem.
I plan to make this better in a future patch, I am investigating the net
benefits of tracking and uniquing template argument lists, much like how we do
for types.
Then, I think we can get rid of this duplication in all places that are just
passing the lists around.
Otherwise, llvm-compile-time-tracker shows no performance impact from this
patch at all (and the same for all the others in the sugared substitution
series), so I did not want to block everything on that extra work.
================
Comment at: clang/include/clang/Sema/TemplateDeduction.h:94
- /// Take ownership of the deduced template argument list.
- TemplateArgumentList *take() {
- TemplateArgumentList *Result = Deduced;
- Deduced = nullptr;
+ /// Take ownership of the deduced template argument lists.
+ TemplateArgumentList *takeSugared() {
----------------
erichkeane wrote:
> Not your fault at all.... but that seems to be about the absolute opposite of
> what it looks like this function is doing...
Hah, it's written in the perspective of the user, not the provider.
The opposite of standard practice I guess.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4876
Context, NamedConcept->getDeclContext(), NamedConcept->getLocation(),
- Converted);
+ CanonicalConverted);
ConstraintSatisfaction Satisfaction;
----------------
erichkeane wrote:
> At one point I wonder if there is value to storing the sugared version here
> instead. We currently just create these every time we need them, so the
> side-effect would be possibly a nicer looking AST dump. BUT we don't really
> canonizalize these Specialization Decls/Specialization Exprs.
That will happen in D136566, which you already reviewed.
If we did that before the changes in D134604, then this would lead to a crash
for certain test cases involving partial substitutions.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133874/new/
https://reviews.llvm.org/D133874
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
