AaronBallman wrote: > > I think we need to understand what we want before we can make decisions on > > what needs changing, though. Are there invariants we want to introduce, > > like the source range for the AST node should encompass the source > > locations tracked within the AST node? Or are we fine with the AST node > > tracking source locations which exist outside of the source range for the > > node itself? How do we want users of the AST to understand what the source > > range represents? > > I think semantically connecting all the requirements of source tools to AST > nodes is difficult. > > > I am currently leaning towards the idea that the source range for an AST > > node should be the union of the ranges tracked by the node and its children. > > I think this makes sense and sounds principled on paper, but achieving that > isn't feasible without breaking users every time we take a step towards it.
Agreed, but any changes to source location fidelity will break users unless it's exposing a new source location we didn't previously expose. Tooling gets ABI stability guarantees, it does not get behavioral stability guarantees; we still need to be able to evolve the compiler as languages and the state of the art move forward. > > Someday, if we start tracking source locations for the storage class > > specifiers, the range may expand to include those. > > I think that would be a breaking change for downstream consumers once again. > People will rely on those changes not being part of the outer decls, and when > we expand them, we'll regress those (e.g. tools that just rewrite types, will > start dropping storage specifiers all of a sudden). > > I really don't know the answer here, and the more I think the harder it > feels. Same! > Even if we didn't have this "incremental issues result in regressions for the > users" issue, as you've also pointed out, I don't think modelling AST nodes > as "properly nested" works for C++ :/ Ignoring macros, pragmas, comments; > declarator syntax itself means type and name of a declaration might be > nested, instead of being siblings. Yeah, that's a fair point. > So I'd probably just keep the source range definition for composite > definitions as-is today, claim them deprecated and just provide "self" > locations for ast nodes, possibly making it multiple locations to model any > discontinuities. e.g. a `VarDecl` would only point to its `name`. you'd need > to drill down into specific parts of it if your tool is interested in the > rest. This sounds like a much simpler contract, and can be incrementally > implemented as well. Moreover it will be stable, no matter what changes we > make to AST, as long as an AST node is there, source location associated with > it will stay there. I think this may be the most principled way forward, but there are some moving parts. This means we need to track a lot more source locations in the AST, which comes with overhead. We don't know the impacts of that increased overhead, some of it may be too painful for us to want to bear. Once we've started tracking enough source location information for an AST node that we no longer need to track the range, we can deprecate the internal range API (since we now have something else we can switch to). Once we've replaced all the range uses with the source location uses, we can get rid of the range API for that node. But this leaves the question about what to do for tooling. We have `clang_getCursorExtent()` (and others) as exposed APIs. We promise people ABI stability, so deprecating an API in libclang is kind of an academic exercise because we can't remove the interface. So do we mark it as deprecated and update the comments to explain it no longer returns a range, just a single location? Or do we try to put range logic into libclang to try to keep some of the cursors limping along with the previous behavior? Something else? https://github.com/llvm/llvm-project/pull/133107 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
