alexander-shaposhnikov 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:
> > > > > 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 ?
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
