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:
> 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.
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
