mizvekov added a comment.

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

> Haha. Pun intended? :-)

Yes :-)

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

I think for IWYU getCanonicalType could be problematic. It would be fine for 
quickly testing what kind of node you have under all that sugar and such, but 
if for example you try to get a Decl represented by some canonical type, you 
would likely get a canonical decl, but it seems to me that would not be useful 
because you might need to know the exact (re)-declaration used, which has 
source location information and you could pin down to a specific file.

There is `getDesugaredType`, which will just pull off all top level sugar 
without really canonicalizing the whole thing.

If instead you want to search down a type for the first thing of interest, then 
you can in that case have a main switch case on the type class, or even perhaps 
keep an if else chain of dyn_casts, but on your default or else case you could 
just test that the current type node is sugar with `getSingleStepDesugaredType` 
on it, see if you get a different result and try again with it in that case.
That way this mechanism does not get poisoned by clang introducing some new 
type sugar, which could not even be relevant to IWYU.

> Thanks!

Thank you for your patience as well, and sorry for the trouble!


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

Reply via email to