[PATCH] D39050: Add index-while-building support to Clang
Eric Liu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 10 05:58:44 PST 2017
ioeric added a comment.
>> - 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.
I also tried applying your original patch locally but couldn't get it to work mostly due to portability issues (e.g. `blocks` and `if (APPLE)` in make files). AFAIK, many folks compile clang with GCC and/or without APPLE, so it's important that you get the portability right from the very beginning. Thanks!
Index-while-build is awesome! I'm looking forward to your patches!
More information about the cfe-commits