================
@@ -1185,7 +1154,7 @@ static bool CheckConstraintSatisfaction(
return false;
}
- if (TemplateArgsLists.isAnyArgInstantiationDependent(S.Context)) {
+ if (TemplateArgsLists.isAnyArgDependent(S.Context)) {
----------------
mizvekov wrote:
> Maybe just `C.getCanonicalTemplateArgument(TA).isDependent()` (which would
> works for expressions, expression canonicalization just returns the
> expression).
For a plain "isDependent" check, `getCanonicalTemplateArgument` buys you
nothing, as an AST node will be dependent 'iff' the corresponding canonical
node is dependent. It costs performance for no benefit, because canonicalizing
TemplateNames is not cheap.
> But it is possible I misunderstood. In particular this does not address
> Matheus' examples.
But I think these examples are unrelated as they are incorrectly compiled with
or without the patches Younan has been working on.
There is a misunderstanding here, #183010 does cause the regression from the
second example (where the concept template parameter is used).
That's not surprising, I am not presenting a random example here for no reason,
this example came up from analyzing the changes and thinking about the
consequences of it.
The change in #183010, even including the changes from this PR, are incorrect
because of the reasons I already explained before, the existing code was
already doing the right thing.
The cache is not the cause here, and it's not a general problem with 'sugars',
the problem is fundamentally that you cannot perform constraint satisfaction
checking if any of the arguments involved contain unsubstituted template
parameters. That's because you don't know if the types involved will become
invalid.
This is specially relevant for concepts because they trap substitution failures
and turn them into a 'false' result.
And for better or for worse, that's part of their design intent.
And that's exactly why the original code was like that, it was deferring
satisfaction checking if any template arguments were instantiation dependent
(ie they contain unsubstituted template parameters, even if that doesn't change
clang's idea of canonical types).
The patch changes that without really explaining what it's doing. From looking
at the patch, it 'gives the impression' that the original code was wrong all
along.
I think the main objections here are:
* We must be careful to document where we have made tradeoffs and workarounds,
to the best of our knowledge. This includes putting the FIXMES in the right
places (ie in the lines changed in #183010, not blaming the cache).
* We have a policy to avoid backports which introduce new regressions. I'd
escalate if we want to make an exception here. Certainly at least the people
involved in managing the stable release should not be left in the dark about
this.
https://github.com/llvm/llvm-project/pull/185608
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
