hokein added inline comments.
================
Comment at: clang/include/clang/AST/Type.h:4499-4508
/// Internal representation of canonical, dependent
/// decltype(expr) types.
///
/// This class is used internally by the ASTContext to manage
/// canonical, dependent types, only. Clients will only see instances
/// of this class via DecltypeType nodes.
+class DependentDecltypeType : public DecltypeType {
----------------
rsmith wrote:
> Can we remove this class now?
yeah I think it is possible, but I would rather do it in a follow-up patch --
the current patch seems large enough. Added a FIXME.
================
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:
> hokein wrote:
> > 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.
> >
> The problem with the patch as it now stands is that for a case such as:
>
> ```
> int x;
> template<typename T> void f(decltype(sizeof(T.f(x))));
> template<typename T> void g(decltype(sizeof(T.f(x))));
> ```
>
> ... there is only one reference to `x` reachable in the AST, on line #2. The
> expression containing the `DeclRefExpr` on line #3 was discarded entirely. So
> (for example) any tooling that's looking to find references to `x` will not
> find the one on line #3.
>
> The best way to fix this would be to store the `Expr*` for the actual
> expression in the `DecltypeTypeLoc` information. We'll also need to make sure
> that `TreeTransform` uses that `Expr*` rather than the one from the type when
> transforming a `DecltypeType` with source info.
Thanks, now I get it.
I have updated this patch to implement what you suggested (storing the actual
expression in `DecltypeTypeLoc`), it touches more files. Please take another
look.
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
