davrec added inline comments.
================ Comment at: clang/lib/AST/ASTContext.cpp:12767 + Ctx.getQualifiedType(Underlying), + ::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl())); + } ---------------- 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. 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
