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

[Kim Gräsman via Phabricator via libcxx-commits]

kimgr added a comment.

> It's the difference in knowing the type was written without any tag or nested-name specifier, and having a type that you are not sure how it was written.
>
> When we are dealing with a type which we are not sure, we would like to print it fully qualified, with a synthetic nested name specifier computed from it's DC,
> because otherwise it could be confusing as the type could come from somewhere very distant from the context we are printing the type from. We would not
> want to assume that a type which has been desugared was written how it's desugared state would seem to imply.

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".

> FWIW, in the state of affairs we leave clang after this patch, I don't think it's worth keeping a separate ElaboratedType anymore, we might as 
> well fuse it's functionality into the type nodes which could be wrapped in it. Taking care to optimize storage when not used otherwise, I think
> we can recoup the performance lost in this patch, perhaps even end in a better state overall.
>
> But I think doing these two steps in one go would not be sensibly incremental. We have in this patch here a very simple core change, which
> is very unlikely to have bugs in itself, but creates enormous test churn.
>
> The second step of eliminating ElaboratedType could be a less simple core change with almost zero test churn, which makes it less risky that
> it would introduce a bug that escapes review.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374