TL;DR: I’d really like to extract the part that goes from AST node → Decl + Role, but I don’t know how to do it incrementally. Current plan would be to copy it to something parallel, and maybe try to port libIndex later. That sounds likely to end in permanent duplication though. Help!
I think it would be great if the current implementation was modularized in a way that would allow Clang and related tools to remove recursive tree visitors that are either abused for other purposes, or just duplicate work that some other visitors are doing, either partially or fully.
I think starting out with a new implementation is not a bad approach. That said, I think it would be really good if the new implementation it was adopted by libIndex from the start, as this would allow:
- A clear path towards convergence of the new implementation and the old implementation
- More coverage for testing
The primary risk of not integrating this into libIndex directly is the high cost of transition to a unified implementation at a later stage by other adopters. Also, there’s a real downside of not unifying code from the start (which we know too well unfortunately ): the general build and code size cost of additional tree visitor code in Clang and its tools.
I think while it’s desirable to change implementation in an incremental way from the perspective of reviews and code history, sometimes it’s not entirely practical as it’s actually harder to see the full picture, and to qualify the change by doing additional testing by other adopters. I think for a change like this a patch that entirely rewrites a lot of the code in libIndex is acceptable. So my suggestion would be to:
- Work on unified implementation with libIndex from the start
- Migrate libIndex in one patch
- The reviewers or you might find ways to split up the patch into smaller, incremental changes during the review
- The reviewers will be able to test one patch only