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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
