[PATCH] D87349: [clang] adapt c++17 behavior for dependent decltype-specifiers

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/AST/Type.cpp:3429
            toTypeDependence(E->getDependence()) |
-               (E->isInstantiationDependent() ? TypeDependence::Dependent
-                                              : TypeDependence::None) |
+               (E->isTypeDependent() ? TypeDependence::Dependent
+                                     : TypeDependence::None) |
----------------
sammccall wrote:
> isn't this redudant with toDependence(E->getDependence)?
I think I must have missed to reset the dependent bit from the result of `toTypeDependence`. `toTypeDependence` will set the dependent bit if `E` is value-dependent, but it is wrong for `decltype`.


================
Comment at: clang/test/CodeGenCXX/mangle.cpp:805
 
-  // CHECK-LABEL: define weak_odr void @_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE
+  // CHECK-LABEL: define weak_odr void @_ZN6test341fIiEEvm
   template void f<int>(decltype(sizeof(1)));
----------------
rsmith wrote:
> rsmith wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > GCC mangles this as `_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE`, so we're breaking compat here :-\
> > > > 
> > > > And in fact we're just incorrect. https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling:
> > > > 
> > > > > If the operand expression of decltype is not instantiation-dependent then the resulting type is encoded directly.
> > > > 
> > > > Here, instantiation-dependent *is* explicitly defined as "type-dependent or value-dependent or ...". And therefore these cases must not be "encoded directly", including the motivating case (decltype(N) where N is a typed constant whose initializer is value-dependent).
> > > > 
> > > > We could consider not two but *three* types of decltypetypes:
> > > >  - decltype(type-dependent), which is sugar for a canonical DependentDeclTypeType
> > > >  - decltype(instantiation-but-not-type-dependent), which is still sugar for a concrete canonical type (per C++17) but mangles using the expr (per cxxabi)
> > > >  - decltype(non-dependent), which is sugar for a concrete canonical type and mangles directly
> > > > 
> > > > This only works if it's OK for mangling to depend on non-canonical type details - I don't know whether this is the case. @rsmith - any hints here?
> > > Hmm, my naive reading of the mangle code matches what I described.
> > > 
> > > The big comment in ItaniumMangle talks about related issues: https://github.com/llvm/llvm-project/blob/24238f09edb98b0f460aa41139874ae5d4e5cd8d/clang/lib/AST/ItaniumMangle.cpp#L2541-L2572
> > > 
> > > I don't really understand what's going on here, sorry.
> > Looks like we need the single-step-desugaring loop below the big comment you quoted to stop when it hits a `decltype` type. That's presumably where we step from the instantiation-dependent-but-not-type-dependent `decltype` node to its desugared type.
> Also... the FIXME from that comment will apply here too. Given:
> ```
> template<typename T> void f(decltype(sizeof(T)), decltype(sizeof(T))) {}
> template void f<int>(unsigned long, unsigned long);
> ```
> we currently (correctly, as far as I can see) use a substitution for the second parameter type, but with the change here, I don't think we will do so any more. We could fix that by deduplicating `DecltypeType`s when they're instantiation dependent but not dependent; that's what we do for `ConstantArrayType`'s size expression, for example. If we do that, we'll need to store the actual expression on the `DecltypeTypeLoc`, since we can't rely on the `DecltypeType` having the right expression any more (only an expression that's equivalent to it).
Thanks for the hints and explanations.

> we currently (correctly, as far as I can see) use a substitution for the second parameter type, but with the change here, I don't think we will do so any more.

thanks for the example, yeah, the behavior was changed with the prior change of this patch. 

> We could fix that by deduplicating DecltypeTypes when they're instantiation dependent but not dependent; that's what we do for ConstantArrayType's size expression, for example. If we do that, we'll need to store the actual expression on the DecltypeTypeLoc, since we can't rely on the DecltypeType having the right expression any more (only an expression that's equivalent to it).

It took me sometime to understand the whole piece here, but I'm afraid that I still don't fully understand the second part -- any reason why we can't rely on the underlying expression of DecltypeType when doing the deduplication? If we store the actual expression on `DecltypeTypeLoc`, how do we retrieve it in `ASTContext::getDecltypeType()`, I didn't find any connection between `DecltypeTypeLoc` and `ASTContext::getDecltypeType`.

I updated the patch using the `DecltypeTypeLoc` to do the duplication, it passes the example you gave above, but it might be wrong. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87349