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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130308/new/

https://reviews.llvm.org/D130308