davrec added inline comments.
================ Comment at: clang/lib/AST/ASTContext.cpp:12767 + Ctx.getQualifiedType(Underlying), + ::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl())); + } ---------------- mizvekov wrote: > davrec wrote: > > mizvekov wrote: > > > davrec wrote: > > > > This last argument should probably be omitted/nullptr passed, since > > > > # We probably won't encounter any owned tag decls in the types passed > > > > to this function; > > > > # Even if we did, it would not necessarily be true that they both have > > > > non-null owned tag decls, and I don't see any nullptr checks in > > > > getCommonDecl; and > > > > # Even if we checked for nullptr there, and say passed the same > > > > argument to X and Y so EX==EY, and that had an owned tag decl, it is > > > > not clear to me it would be appropriate to construct a new type with > > > > the same owned tag decl as another type. > > > 1. We can see, a one liner like this will do: `int e = true ? (struct > > > S*)0 : (struct S*)0;` > > > > > > Though normally in an example like the above, you would only see the > > > owned decl on X, because when we get to Y the name has already been > > > introduced into the scope. > > > > > > I have an idea for a test case, but it's a bit convoluted: > > > > > > This also introduces an OwnedTagDecl: `(struct S<T>*)0`. > > > > > > So with resugaring, if we resugar T, it might be possible to construct > > > this situatiation. Given if it's appropriate to keep the OwnedTagDecl > > > when resugaring, of course, which goes to 3) > > > > > > 2. This is handled by `declaresSameEntity`, if either or both decls are > > > null, we say they are not the same decl. > > > > > > 3. That I am not sure. I see that this OwnedTagDecl is only used by the > > > type printer, and it replaces printing the rest of the type by printing > > > the OwnedDecl instead. So why do you think it would not be appropriate > > > that the rebuilt type keeps this? > > (Re #3) Because I think the sense ownership is respected in how > > ownedTagDecls are presently handled: at most one ElaboratedType owns a > > particular TagDecl, *and* that type does not seem to be reused elsewhere in > > the AST. (This is relied on in https://reviews.llvm.org/D131685, which was > > on my mind when I looked at this.) > > > > E.g. consider this expansion of your example: > > ``` > > auto e = true ? (struct S*)0 : (true ? (struct S*)0 : (struct S*)0); > > ``` > > The first ElaboratedType has an ownedTagDecl; the second is a distinct > > ElaboratedType without an ownedTagDecl, and the third equals the second. > > > > Now in practice what this means is that `getCommonDecl` when used on binary > > expressions of this sort will always be nullptr, so no harm done. But > > suppose it is called with X=Y=some ET with an ownedTagDecl (which is the > > only time I believe we could see a non null getCommonDecl result here). > > Should the type that is returned have that same owned tag decl? I think > > that would violate the sense of the original type "owning" that decl that > > seems to be respected elsewhere in the AST. > > > > Re type printing, I believe that ownedTagDecls only affects printing of > > defined TagDecls like `struct S { int i; } x;` (which is what I was mostly > > thinking re ownedTagDecls - I don't think we will see those here); for > > undefined ones like `struct S x;`, printing should be unaffected whether > > there is an ownedTagDecl or not. > > > With type deduction this can happen: > > ``` > auto x = (struct S*)0; // it will also appear in the type of x > using t = decltype(x); // and now also in t > ``` > > With your second example, it can also happen: > > ``` > struct S { int i; } x; > int e = true ? &x : (struct S*)0; > ``` > > In those cases, the type node that owns the decl will be the same object, but > that is only because of uniquing. > > It may be possible that two different objects end up in this situation, if > for example they come from different modules that got merged. > With type deduction this can happen: > ``` > auto x = (struct S*)0; // it will also appear in the type of x > using t = decltype(x); // and now also in t > ``` My local clang is out of date - the type of `x` for me is just an AutoType wrapping a PointerType to a RecordType. In light of the addition of the ElaboratedType there, it seems fine to be consistent here with however that case handles ownedTagDecls, but FWIW I do not think that deduced ElaboratedType should have an ownedTagDecl either. Only the original type should be the owner - not any type deduced from it. This is a minor issue with few if any observable effects for now, but should be kept in mind if issues arise later: the fact the ownership of ownedTagDecls is respected was the only factor that made https://discourse.llvm.org/t/ast-parent-of-class-in-a-friend-declaration/64275 easily solvable. > With your second example, it can also happen: > > ``` > struct S { int i; } x; > int e = true ? &x : (struct S*)0; > ``` The DeclRefExpr `x` indeed reuses the ElaboratedType used in the VarDecl `x`, with its ownedTagDecl and all. That seems fair, a special case. For all other type deductions I would mildly object to including an ownedTagDecl. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130308/new/ https://reviews.llvm.org/D130308 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
