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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
