[PATCH] D39050: Add index-while-building support to Clang

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D39050#922597, @akyrtzi wrote:

> Hey Eric,
>
> In https://reviews.llvm.org/D39050#921748, @ioeric wrote:
>
> > >> - I think the implementation of the index output logic (e.g. `IndexUnitWriter` and bit format file) can be abstracted away (and split into separate patches) so that you can unit-test the action with a custom/mock unit writer; this would also make the action reusable with (potentially) other storage formats.
> > > 
> > > The added IndexRecordAction and existing IndexAction use the same functionality from libIndex to collect the indexing data, so I'm not sure mocking the unit writer to unit test IndexRecordAction would add very much value – writing the index data out is the new behavior. The existing tests for IndexAction (under test/Index/Core) are already covering the correctness of the majority of the collected indexing info and the tests coming in the follow-up patch (seen in the original version of this patch) test it's still correct after the write/read round trip.
> >
> > Thanks for the clarification! I still think it's useful to decouple the IndexAction from the bit format file so that it could be reusable elsewhere. For example, I can see the index action be useful to clangd for building in-memory index.
>
>
> As Nathan mentioned, we believe the indexing action, as it exists in the trunk, is decoupled enough to be useful, for example Marc was already able to use it and write out the indexing data in a completely different format for his fork of clangd. Of course, we are definitely interested in any additional refactorings that would structure things better and we are eager to see and discuss follow-up patches from anyone that is interested in improving the code, but could we treat this as potential follow-up improvements ?
>
> We are eager to provide the functionality so others can start experimenting with it; I'd propose that we discuss ideas for refactoring of the code as follow-up, what do you think ? Getting the initial functionality in and iterating on it, while getting more experience with applying it on various use-cases, is a common operating mindset of the llvm/clang projects.


To be honest, I want this functionality to get in as much as you do, and I'm more than happy to prioritize the code review for it :) But the current patch size makes the reviewing really hard (e.g. I would never have caught the BLOCK issues hadn't I tried running the original patch myself). I'm not sure if it's really a common practice to check in a big chunk of code without careful code review and leave potential improvements as followups. I'm sure @klimek would have thoughts about this.

If the index action is already flexible enough, would you mind splitting the code for the index action out so that we can start reviewing it? Given that the current patch has very few tests, I guess it wouldn't be too much worse to split out the action without proper test.


https://reviews.llvm.org/D39050