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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D146178#4213943 <https://reviews.llvm.org/D146178#4213943>, @alexander-shaposhnikov wrote:

> @erichkeane - thanks for the comments, the changes in SemaTemplateInstantiateDecl.cpp are necessary, in particular, they enable us to handle the case
>
>   template <class T0>
>   concept Constraint = true;
>   
>   template <Constraint T1>
>   struct Iterator {
>     template <Constraint T2>
>     friend class Iterator;
>   };
>   
>   Iterator<int*> I;
>
> (and the negative one) 
> with the same machinery.
> Regarding SFINAE  - it's necessary as well, 
> the failed substitution is considered as "not equal" (according to the standard (mentioned by Richard above)).

Yeah, I think you're right about the SFINAE bit.  I'll have to think on that example a while.  Hopefully Richard can stop by and comment on this instead, I'm not sure I completely get the how those changes work.  One thing I DID end up doing that I forgot to mention, was :

  --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
  +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
  @@ -208,6 +208,10 @@ Response HandleFunction(const FunctionDecl *Function,
     return Response::UseNextDecl(Function);
   }
  
  +Response HandleFunctionTemplateDecl(const FunctionTemplateDecl *FTD) {
  +  return Response::ChangeDecl(FTD->getLexicalDeclContext());
  +}
  +
   Response HandleRecordDecl(const CXXRecordDecl *Rec,
                             MultiLevelTemplateArgumentList &Result,
                             ASTContext &Context,
  @@ -318,6 +322,8 @@ MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(
       } else if (const auto *CSD =
                      dyn_cast<ImplicitConceptSpecializationDecl>(CurDecl)) {
         R = HandleImplicitConceptSpecializationDecl(CSD, Result);
  +    } else if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(CurDecl)) {
  +      R = HandleFunctionTemplateDecl(FTD);
       } else if (!isa<DeclContext>(CurDecl)) {
         R = Response::DontClearRelativeToPrimaryNextDecl(CurDecl);
         if (CurDecl->getDeclContext()->isTranslationUnit()) {
  @@ -3949,16 +3955,16 @@ Sema::SubstExpr(Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs) {
   }

Which was necessary for properly getting the template argument lists for the out of line version, so I'm a little shocked/surprised it worked without it (else, the MLTAL for an out of line definition will include the struct's template arguments, which seems wrong).

I think the example you included there needs to go into the tests, as does the one that Richard provided.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178