rsmith added inline comments.
================
Comment at: clang/include/clang/AST/Type.h:4478
- DecltypeType(Expr *E, QualType underlyingType, QualType can = QualType());
+ const ASTContext &Context;
+ DecltypeType(Expr *E, QualType underlyingType, const ASTContext &Context,
----------------
We shouldn't store the context here. To provide it to the `Profile`
implementation, use a `ContextualFoldingSet`.
================
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 {
----------------
Can we remove this class now?
================
Comment at: clang/lib/AST/ASTContext.cpp:5342-5347
/// Unlike many "get<Type>" functions, we don't unique DecltypeType
/// nodes. This would never be helpful, since each such type has its own
/// expression, and would not give a significant memory saving, since there
/// is an Expr tree under each such type.
+/// The instantiation-dependent-but-not-type-dependent DecltypeType node is an
+/// exception, we unique it for forming correct substitutions in name mangling.
----------------
Maybe fold these two paragraphs together
> Unlike many "get<Type>" functions, we don't unique DecltypeType nodes unless
> they are instantiation-dependent. [...]
================
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)));
----------------
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.
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
