[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Alexander Shaposhnikov via Phabricator via cfe-commits]

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