ioeric added a comment. In https://reviews.llvm.org/D39050#900830, @arphaman wrote:
> I think this patch should be split into a number of smaller patches to help > the review process. > > Things like `tools/IndexStore`, `DirectoryWatcher` and other components that > are not directly needed right now should definitely be in their own patches. > It would be nice to find some way to split the implementation into multiple > patches as well. +1. This is a lot of work (but great work!) for one patch. Smaller/incremental patches help reviewers understand and (hopefully) capture potential improvement of the design. I would really appreciate it if you could further split the patch. Some comments/ideas: - The lack of tests is a bit concerning. - 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. - I would suggest that you start with a patch that implement the index action and just enough components so that you could test the action. Thanks! https://reviews.llvm.org/D39050 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
