[libcxx-commits] [PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

Sat Jul 16 14:14:56 PDT 2022

mizvekov added a comment.

In D112374#3657472 <https://reviews.llvm.org/D112374#3657472>, @kimgr wrote:

> I'm coming at this from pretty far away, so there's very likely lots of details that I'm overlooking. But it seems to me the mainline had only had an `ElaboratedType` node if there was elaboration, and not otherwise. And that makes a lot more sense to me than having 2 `ElaboratedType*` nodes _for every type in the AST_, just to express that "hey, by the way, this type had no elaboration".

There are no 2 `ElaboratedType` nodes, there is only one. If you are seeing something like an ElaboratedType wrapping directly over another ElaboratedType, that would seem to be a bug.

To the second point, it's a problem of representation. Having no elaboration is not the same thing as having no information about elaboration, so we better not represent both things with the same state.

> That sounds good at face value, but if you're planning to remove these nodes again, that would create enormous churn for out-of-tree tools to re-adjust to the new shape of the tree.
>
> I can't say what the best solution is, but this patch generates quite a lot of work for me, and I would really hope that catching up with the new AST does not generate even more work down the line.

That part I don't understand why. Before this patch, clang can produce a bunch of type nodes wrapped in an ElTy, or not. After this patch, we add ElTys in more cases, but the basic situation remains the same.

Why IWYU would even care about ElaboratedTypes at all? I would have expected a `git grep ElaboratedType` on IWYU sources to have no matches.

Can you elaborate on that?

In general, I would not expect external tools to care about the shape of the AST. I would expect the type API would be used in a way where we ignore a type sugar node we have no reason to acknowledge.
Ie you query if some type is a (possible sugar to) X, and you would either get X or nothing. The type sugar over it would just be skipped over and you would have no reason to know what was in there or what shape it had.

Of course that was not what happened in practice. A lot of code used `dyn_cast` where it should have used `getAs`. Just look at all the fixes in this patch for examples.
And fixing that, besides making that code compatible with this patch, also fixed other bugs where it would not properly ignore other pre-existing type sugar.

If IWYU has unit tests that test too much clang implementation details, that would generate unneeded burden on both sides.  Is it for example doing AST dump tests and expecting exact outputs?
Not even the clang test suite does that too much.
Is it to compensate for any perceived lack of testing on mainline side?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374