[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/SemaTemplateInstantiate.cpp:133
 }
 
+Response HandlePartialClassTemplateSpec(
----------------
alexander-shaposhnikov wrote:
> alexander-shaposhnikov wrote:
> > alexander-shaposhnikov wrote:
> > > alexander-shaposhnikov wrote:
> > > > HandlePartialClassTemplateSpec is from Erich's diff (https://reviews.llvm.org/D147722)
> > > @erichkeane : previously (see https://reviews.llvm.org/D147722) we were producing a real layer of arguments (if the depth is greater than 1), to the best of my knowledge this is incorrect since it'd trigger unexpected depth adjustment, we should produce retained layers instead.
> > (depth adjustment during the substitution)
> to make it a bit easier to reason about I've changed the names in the test case:
> 
> ```
> //enum class Enum { E1 };
> 
> template <typename T1>
> concept some_concept = true;
> //inline constexpr bool some_concept = true;
> 
> template <typename T1, int>
> struct S {
>   template <typename T2>
>   requires(some_concept<T2>)
>   void func(const T2 &);
> };
> 
> template <typename T11>
> struct S<T11, 0> {
>   template <typename T22>
>   requires(some_concept<T22>)
>   void func(const T22 &);
> };
> 
> template <typename T111>
> template <typename T222>
> requires (some_concept<T222>)
> inline void S<T111, 0>::func(const T222 &) {}
> ```
> 
> Previously the code was doing the following:
> ```
> PartialClassTemplSpec->getTemplateDepth() = 1
> NumRetainedOuterLevels: 0
> 0: <>
> before subst:
> ParenExpr 0x558fb90be6a8 '_Bool'
> `-ConceptSpecializationExpr 0x558fb90be640 '_Bool' Concept 0x558fb909fbd8 'some_concept'
>   |-ImplicitConceptSpecializationDecl 0x558fb90be5d0 
>   | `-TemplateArgument type 'type-parameter-1-0'
>   |   `-TemplateTypeParmType 0x558fb90a01d0 'type-parameter-1-0' dependent depth 1 index 0
>   `-TemplateArgument type 'T22'
>     `-TemplateTypeParmType 0x558fb90be590 'T22' dependent depth 1 index 0
>       `-TemplateTypeParm 0x558fb90be540 'T22'
> after subst:
> ParenExpr 0x558fb90bf168 '_Bool'
> `-ConceptSpecializationExpr 0x558fb90bf100 '_Bool' Concept 0x558fb909fbd8 'some_concept'
>   |-ImplicitConceptSpecializationDecl 0x558fb90bf090 
>   | `-TemplateArgument type 'type-parameter-0-0'
>   |   `-TemplateTypeParmType 0x558fb909fb50 'type-parameter-0-0' dependent depth 0 index 0
>   `-TemplateArgument type 'T22'
>     `-TemplateTypeParmType 0x558fb90bf050 'T22' dependent depth 0 index 0
>       `-TemplateTypeParm 0x558fb90be540 'T22'
> ```
> (as one can see the depths have changed)
> 
> now:
> ```
> PartialClassTemplSpec->getTemplateDepth() = 1
> NumRetainedOuterLevels: 1
> before subst:
> ParenExpr 0x55f7813ae6a8 '_Bool'
> `-ConceptSpecializationExpr 0x55f7813ae640 '_Bool' Concept 0x55f78138fbd8 'some_concept'
>   |-ImplicitConceptSpecializationDecl 0x55f7813ae5d0 
>   | `-TemplateArgument type 'type-parameter-1-0'
>   |   `-TemplateTypeParmType 0x55f7813901d0 'type-parameter-1-0' dependent depth 1 index 0
>   `-TemplateArgument type 'T22'
>     `-TemplateTypeParmType 0x55f7813ae590 'T22' dependent depth 1 index 0
>       `-TemplateTypeParm 0x55f7813ae540 'T22'
> after subst:
> ParenExpr 0x55f7813af138 '_Bool'
> `-ConceptSpecializationExpr 0x55f7813af0d0 '_Bool' Concept 0x55f78138fbd8 'some_concept'
>   |-ImplicitConceptSpecializationDecl 0x55f7813af060 
>   | `-TemplateArgument type 'type-parameter-1-0'
>   |   `-TemplateTypeParmType 0x55f7813901d0 'type-parameter-1-0' dependent depth 1 index 0
>   `-TemplateArgument type 'T22'
>     `-TemplateTypeParmType 0x55f7813ae590 'T22' dependent depth 1 index 0
>       `-TemplateTypeParm 0x55f7813ae540 'T22'
> ```
> 
> 
So the thought behind the 'empty' level was for a case where the `PartialClassTemplSpec` is defined inside of another type that actually DOES have arguments we need to compare.  Though, I guess for the purposes of substitution, it isn't really needed (since if we have a `PartialClassTemplSpec`, we can't fully instantiate ANYWAY)... and I can't think of a case where it totally matters anyway.  I think I'm OK with this as is, but I'm a touch suspicious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178