nathawes planned changes to this revision. nathawes added a comment. In https://reviews.llvm.org/D39050#920451, @ioeric wrote:
> 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. Thanks for taking a look @ioeric! I'll have a go at splitting it further. > Some comments/ideas: > > - The lack of tests is a bit concerning. I moved all the code for reading the index store data into a separate patch (to come after this one) in order to slim this one down for review, and most of the tests went with it because they're based around reading and dumping the stored data for FileCheck. The original version of this patch has them all (https://reviews.llvm.org/D39050?id=118854). The ones that remain here are just those checking that the unit/record files are written out and that the hashing mechanism is producing distinct record files when the symbolic content of the source file changes. > - 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. > - 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
