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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to