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

[Erich Keane via Phabricator via cfe-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;
----------------
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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178