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

[Erich Keane via Phabricator via llvm-commits]

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146178/new/

https://reviews.llvm.org/D146178