alexander-shaposhnikov planned changes to this revision.
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:
> erichkeane wrote:
> > 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`.
> To answer this question:
>
> ```
> namespace DelayedInst {
> template<unsigned I>
> struct AAA {
> template<typename T>
> requires (sizeof(T) == I)
> struct B {
> static_constexpr int a = 0;
> };
>
> static constexpr auto foo() {
> return B<int>::a;
> }
> };
> ```
> Ends up instantiating `B<int>` to get `a` despite `I` not being filled in
> yet, but we still try instantiation into the requires clause for various
> syntactic checking. So we need something to take up the space of the `I`.
> We cant just do the retained layer, since `AAA` might be inside of ANOTHER
> class template (perhaps a specialization?), so we'd need that one as well.
>
> So that code goes to try to introduce the `I`. So I think this solution ends
> up coming down to 'how do we get `T4` into `foo3`'s MLTAL.
Regarding ForConstraintInstantiation=true - I agree,
i think i got confused by a particular example i was looking at.
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
