akyrtzi added a comment. 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. > 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! Nathan will look into making using blocks optional, providing additional function pointer+context APIs where appropriate and having the common implementation using lambdas. For the APPLE specific parts it, the only specific darwin-specific part is the part using FSEvents, the other 'if (APPLE)' checks can likely be removed. We would generally need help from people with linux expertise to provide the 'FSEvents' equivalent functionality but this is a small part of the overall feature, it's not important for getting the index-while-building data. But these things are not part of the current patch, we can discuss again with the follow-up patches that will contain those things. > Index-while-build is awesome! I'm looking forward to your patches! https://reviews.llvm.org/D39050 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
