ilya-biryukov accepted this revision. ilya-biryukov added a comment. In D66751#1646991 <https://reviews.llvm.org/D66751#1646991>, @sammccall wrote:
> Yes. I actually ripped this out of the patch, because it wasn't related > enough and the patch is large. Makes sense and thanks for summing it up. Just wanted to make sure we are thinking about this use-case. My comment was specifically referring to the lack of a "flat" model in our codebase. I believe hierarchical model is very well handled by the selection tree (there might be bugs, obviously, but conceptually it's doing the right thing). >> E.g. finding a source location of field for DeclRefExpr produced for >> MyBase::field seems to be the same amount of work (in general case) as >> finding the Decl* of field. > > For the hierarchical model it's easy, this is one of the few methods > DynTypeNode actually has :-) > For the flat model: it's not trivial, but is less work because while you do > have to dispatch over all node types, you don't have general multi-hop/graph > cases (I think). > Usually the "handle" tokens are well-supported by the AST because they're > precisely the ones that diagnostics want to report. So in the DeclRefExpr > case, DeclRefExpr::getLoc() returns the right thing. To be clear, the concern is **not** that it's hard to find the corresponding locations for each particular AST node. The concern is that it's hard to cover **all** possible nodes and we should do it once rather than repeat this multiple times. `DeclRefExpr` was a bad example for that purpose. E.g. one could make the same claim about `targetDecl` function we are introducing: it's very easy to get a target decl of `DeclRefExpr`, but it's hard to enumerate all nodes that can have target decls and ensure they are handled properly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66751/new/ https://reviews.llvm.org/D66751 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
