erichkeane added inline comments.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:773
+ // ConstrExpr for the inner template will properly adjust the depths.
+ if (isa<CXXRecordDecl>(ND) && isa<CXXRecordDecl>(OtherND))
+ ForConstraintInstantiation = true;
----------------
erichkeane wrote:
> alexander-shaposhnikov wrote:
> > erichkeane wrote:
> > > alexander-shaposhnikov wrote:
> > > > erichkeane wrote:
> > > > > alexander-shaposhnikov wrote:
> > > > > > erichkeane wrote:
> > > > > > > Hmm... this seems really strange to have to do.
> > > > > > > `ForConstraintInstantiation` shouldn't be used here, the point of
> > > > > > > that is to make sure we 'keep looking upward' once we hit a spot
> > > > > > > we normally stop with. What exactly is the issue that you end up
> > > > > > > running into here? Perhaps I can spend some time debugging what
> > > > > > > we should really be doign.
> > > > > > yeah, I agree. I haven't found a proper solution or at least a
> > > > > > better workaround (but would be happy to).
> > > > > > This kicks in for the case
> > > > > >
> > > > > > ```
> > > > > > template <class T0>
> > > > > > concept Constraint = true;
> > > > > >
> > > > > >
> > > > > > template<Constraint T1>
> > > > > > struct Iterator {
> > > > > > template <Constraint T2>
> > > > > > friend class Iterator;
> > > > > > void operator*();
> > > > > > };
> > > > > >
> > > > > > Iterator<char*> I2;
> > > > > > ```
> > > > > > yeah, I agree. I haven't found a proper solution or at least a
> > > > > > better workaround (but would be happy to).
> > > > > > This kicks in for the case
> > > > > >
> > > > > > ```
> > > > > > template <class T0>
> > > > > > concept Constraint = true;
> > > > > >
> > > > > >
> > > > > > template<Constraint T1>
> > > > > > struct Iterator {
> > > > > > template <Constraint T2>
> > > > > > friend class Iterator;
> > > > > > void operator*();
> > > > > > };
> > > > > >
> > > > > > Iterator<char*> I2;
> > > > > > ```
> > > > >
> > > > > Alright, well, I should have time later in the week to poke at this,
> > > > > perhaps I can come up with something better? I DO remember
> > > > > self-friend is a little wacky, and I spent a bunch of time on it last
> > > > > time.
> > > > Ok, sounds good + maybe Richard will give it another look.
> > > So IMO, `ForConstraintInstantiation` should be 'true' always, and that
> > > makes those examples pass. However, I'm now seeing that it causes a
> > > failure in the concepts-out-of-line-def.cpp file.
> > >
> > > I took the example of `foo3`:
> > >
> > > ```
> > > template<typename T0> concept C = true;
> > > template <typename T1>
> > > struct S {
> > > template <typename F3> requires C<F3>
> > > void foo3(F3 f); // #1
> > > };
> > > template <typename T4>
> > > template <typename F6> requires C<F6>
> > > void S<T4>::foo3(F6 f) {} // #3
> > > ```
> > >
> > > Which, seems to require `ForConstraintInstantiation` to be false to pass.
> > > However, I don't think this is correct. This is only working because
> > > when evaluating the in-line one (#1 above!) its skipping the application
> > > of `T1`, which is wrong.
> > >
> > > However, I think the problem here is that the `out of line` version (#3)
> > > is not applying the T4 like it should be. SO, I think the
> > > `HandleFunctionTemplateDecl` I provided you earlier needs modification.
> > >
> > > FIRST, though not related to this, I think we might need to add
> > > `FunctionTemplateDecl::getInjectedTemplateArgs` to the `Result`, but only
> > > because that 'sounds right' to me? IDK what other problem that would
> > > cause, but it is worth evaluating/saving for later. It might just not
> > > matter, since we're treating them as equal at the moment, I don't think
> > > injecting them would cause anything.
> > >
> > > Secondly: I see that the we can get to the `T4` via the
> > > `FTD->getTemplatedDecl()->getQualifier()->getAsType()->getAs<TemplateSpecializationType>()->template_arguments()`.
> > >
> > >
> > > HOWEVER, the problem that comes with picking THOSE up, is that it ALSO
> > > applies with a `FunctionTemplateDecl` inside of an out-of-line
> > > `ClassTemplateSpecializationDecl` (which doesn't NEED the
> > > specialization's template args).
> > >
> > > SO I think the fix here is something in `HandleFunctionTemplateDecl` to
> > > make sure we pick up the right list of template arguments. I haven't
> > > figured out the magic incantation to make it work unfortunately, perhaps
> > > @rsmith has some hints? Else I'll keep poking at it.
> > >
> > > BUT, my debugging has shown me that I'm quite convinced the setting
> > > `ForConstraintInstantiation ` to false is incorrect.
> > p.s. Richard is on vacation.
> >
> > to quote his comment above (where the story began):
> >
> > >The right way to fix that and the issue being addressed here is that,
> > >rather than adjusting the depths, we ?>should substitute the outer
> > >template arguments from the scope specifier (A<int>::) into the constraint
> > >before >performing the comparison. (In the special case where none of the
> > >outer template parameters are used by the >inner template, that does
> > >effectively just adjust the depths of any inner template parameters.)
> >
> > A. the formal meaning of ForConstraintInstantiation=true/false is unclear,
> > @eirchkean - if you happen to understand it - mind expanding the comment a
> > bit ? (and we can add this to the documentation)
> >
> > B. <related to A> T1 - maybe it should be collected as a retained layer ?
> >
> I also notice that the work in `HandleRecordDecl` inside of the
> `ForConstraintInstantiation` is a little odd, I don't recall why I put that
> in place, but it DOES result in 5 failed tests if I remove it. Perhaps we
> just need to be more careful when we substitute in these arguments? It seems
> to me that putting those arguments in place is perhaps not the right thing to
> do, since they don't have any meaningful values yet? Perhaps something else
> for us to poke at.
Ah, didn't know he was away :)
My understanding is that is what you're doing (the quote). By collecting the
instantiation args correctly, that is what should be happening here.
A:
The `ForConstraintInstantiation`s intent is to make it so we collect arguments
'all the way up' rather than stopping at some points where normal instantiation
would. The idea is that a Constraint is never stored in the AST as
substituted, so the depth is always relative to the TU. Other situations where
we collect arguments is relative to the 'last thing that has been
instantiated'. So thats the difference (is where we are substituting
'relative' to).
B: At this point, we COULD and probably SHOULD collect T1 as a retained outer
layer, but that would still cause a problem with that example. I think the
issue here is that we're not ALSO collecting `T4`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146178/new/
https://reviews.llvm.org/D146178
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
