[libcxx-commits] [PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[David Rector via Phabricator via libcxx-commits]

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