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

Argyrios Kyrtzidis via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Nov 16 15:35:32 PST 2017


akyrtzi added a comment.

> 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.

To be clear, I didn't mean to imply we don't want careful code review, we are really happy for people to point out issues. For example the building problems on linux are serious issues that we will fix and we are grateful for your feedback!

> 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.

To clarify, the index action Nathan and I are referring to, is the indexing action that exists currently in trunk and is the source of the index symbols, feeding index symbols to an abstract `IndexDataConsumer`. See here: https://llvm.org/svn/llvm-project/cfe/trunk/include/clang/Index/IndexingAction.h
This is what Marc used to get the index symbols and store them in his own format. Tests for this functionality are in: https://llvm.org/svn/llvm-project/cfe/trunk/test/Index/Core/


https://reviews.llvm.org/D39050





More information about the cfe-commits mailing list