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

Sun Jul 17 02:08:12 PDT 2022

kimgr added a comment.

In D112374#3657640 <https://reviews.llvm.org/D112374#3657640>, @mizvekov wrote:

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

I meant the `ElaboratedTypeLoc` + `ElaboratedType`, but yeah, those are parallel, not nested.

>> 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?

Haha. Pun intended? :-)

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

As you noticed, it's not our tests that care about the AST, it's the tool itself. IWYU has been around since 2010-11, so there's probably lots of code in there to work around bugs and idiosyncrasies in the Clang AST that have since been fixed. I've inherited the project, so I don't have much information on how or why the implementation ended up the way it did.

Anyway, thanks for the heads-up about `getAs` vs `dyn_cast`, that seems like an easy transformation for us to do. Though I'm not sure where -- everywhere a `Type*` is processed?

Also, I suspect we have a few open-ended searches where we're looking for the first desugared type in the tree, but I guess that's where `getCanonicalType` would be used?

Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374