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

akyrtzi added inline comments. Comment at: include/clang/Frontend/FrontendOptions.h:380 + /// Whether to ignore system files when writing out index data + unsigned IndexIgnoreSystemSymbols : 1; + /// Whether to include the codegen name of symbols in the index data

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

jkorous added a comment. I'll address comments for this patch in the new set of patches. @gribozavr I haven't put up this part of code for the new round of review yet. I will keep this on mind. @mgrang This already landed in edbbe470f66

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

jkorous abandoned this revision. jkorous added a comment. It's time to officially abandon these patches in favor of new push for upstreaming index-while-building. Current reviews in progress https://reviews.llvm.org/D58749 https://reviews.llvm.org/D58418 RFC

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

mgrang added inline comments. Comment at: lib/Index/FileIndexData.cpp:31 + std::vector Sorted(Decls); + std::sort(Sorted.begin(), Sorted.end()); + return Sorted; Please use range-based llvm::sort instead of std::sort: ``` llvm::sort(Sorted); ``` See

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

gribozavr added inline comments. Herald added a subscriber: jdoerfert. Comment at: include/clang/Frontend/FrontendOptions.h:377 + /// The path to write index data to + std::string IndexStorePath; Please end comments with a period. Comment

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

nathawes updated this revision to Diff 153190. nathawes added a comment. Updated to apply on top-of-tree. https://reviews.llvm.org/D39050 Files: include/clang/Basic/AllDiagnostics.h include/clang/Basic/CMakeLists.txt include/clang/Basic/Diagnostic.td

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

malaperle added a comment. In https://reviews.llvm.org/D39050#1040501, @akyrtzi wrote: > > That would be good. How would one go about asking Clang to generate this > > extra information? Would a Clang Plugin be suitable for this? > > That's an interesting idea that we could explore, but I don't

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

akyrtzi added a comment. > That would be good. How would one go about asking Clang to generate this > extra information? Would a Clang Plugin be suitable for this? That's an interesting idea that we could explore, but I don't have much experience with that mechanism to comment on. > Only the

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

malaperle added a comment. In https://reviews.llvm.org/D39050#1037796, @akyrtzi wrote: > Hey Marc, > > > The fact that both clang and clangd have to agree on the format so that > > index-while-building can be used seems to make it inherently not possible > > to extend > > I don't think "not

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

akyrtzi added a comment. Hey Marc, > The fact that both clang and clangd have to agree on the format so that > index-while-building can be used seems to make it inherently not possible to > extend I don't think "not possible to extend" is quite correct, we can make it so that the format

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

nathawes added a comment. In https://reviews.llvm.org/D39050#1037008, @malaperle wrote: > In https://reviews.llvm.org/D39050#1036394, @nathawes wrote: > > > @malaperle Just to clarify, what's the particular end-loc we're talking > > about here? e.g. for a function call, would this be the end of

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

malaperle added a comment. In https://reviews.llvm.org/D39050#1036394, @nathawes wrote: > @malaperle Just to clarify, what's the particular end-loc we're talking about > here? e.g. for a function call, would this be the end of the function's name, > or the closing paren? > For the end of the

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

nathawes added a comment. In https://reviews.llvm.org/D39050#1036249, @malaperle wrote: > In https://reviews.llvm.org/D39050#1021204, @malaperle wrote: > > > For computing the start/end-loc of function bodies, I tried the > > SingleFileParseMode and SkipFunctionBodies separately ( as a start).

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

malaperle added a comment. In https://reviews.llvm.org/D39050#1021204, @malaperle wrote: > For computing the start/end-loc of function bodies, I tried the > SingleFileParseMode and SkipFunctionBodies separately ( as a start). The > source I use this on looks like this: > > Given the

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

malaperle added a comment. In https://reviews.llvm.org/D39050#949185, @malaperle wrote: > In https://reviews.llvm.org/D39050#948500, @akyrtzi wrote: > > > @malaperle, to clarify we are not suggesting that you write your own > > parser, the suggestion is to use clang in 'fast-scan' mode to get

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

nathawes added a comment. @ioeric I'm working on a few other priorities over the next few weeks, sorry, but should get back to this relatively soon after that. I would just land it, but I expect some downstream breakage I want to make sure I have time to fix. @malaperle Sounds good – I'll keep

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

malaperle added a comment. In https://reviews.llvm.org/D39050#1004937, @ioeric wrote: > I'm wondering if there is any further plan for this? ;) I'd like to comment on the amount of data that will be stored but that can be done outside this review. I still have a few things to figure out

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

ioeric added a comment. I'm wondering if there is any further plan for this? ;) https://reviews.llvm.org/D39050 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. Nice! Thanks for making the refactoring and adding tests! I think this is good to go now. I'm not very familiar with code outside of the index library (Driver, Basic etc), but the changes

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

nathawes updated this revision to Diff 130496. nathawes marked 6 inline comments as done. nathawes added a comment. - Applied the various refactorings suggested by @ioeric - Extended c-index-test with a new option to print out the collected unit indexing data, and - Added tests for the unit

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

nathawes planned changes to this revision. nathawes marked 32 inline comments as done. nathawes added a comment. @ioeric I should have an updated patch up shortly with your inline comments addressed + new tests. Thanks again for reviewing! Comment at:

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

ioeric requested changes to this revision. ioeric added a comment. This revision now requires changes to proceed. (I think I forgot to update the patch status :) https://reviews.llvm.org/D39050 ___ cfe-commits mailing list

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

ioeric added inline comments. Comment at: include/clang/Index/IndexUnitDataConsumer.h:1 +//===--- IndexUnitDataConsumer.h - Abstract index unit data consumer ---===// +// ioeric wrote: > IIUC, this is the index data for a translation unit, as

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

ioeric added a comment. Thanks a lot for further cleaning up the patch! It is now much easier to review. I really appreciate it! Some more comments on the public APIs and the layering of classes. There are a lot of helper classes in the implementation, so I think it's important to get a clear

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

nathawes updated this revision to Diff 127568. nathawes added a comment. Fix out of data header comment in FileIndexData.h https://reviews.llvm.org/D39050 Files: include/clang/Basic/AllDiagnostics.h include/clang/Basic/CMakeLists.txt include/clang/Basic/Diagnostic.td

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

nathawes updated this revision to Diff 127412. nathawes marked an inline comment as done. nathawes added a comment. I've refactored the indexing/dependency data collection out from the writing with the new IndexUnitDataConsumer class, and made other smaller changes to address the feedback from

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

nathawes marked 45 inline comments as done. nathawes added inline comments. Comment at: lib/Index/FileIndexRecord.h:51 +public: + FileIndexRecord(FileID FID, bool isSystem) : FID(FID), IsSystem(isSystem) {} + ioeric wrote: > s/isSystem/IsSystem/ > > Also, I

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

nathawes planned changes to this revision. nathawes added a comment. Thanks for taking another look @ioeric – I'll work through your comments and update. https://reviews.llvm.org/D39050 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

ioeric added a comment. Thanks a lot for the changes! Some more comments inlined. Please mark addressed comments as done so that reviewers could know what to look :) Thanks! Comment at: include/clang/Frontend/CompilerInstance.h:187 + typedef std::function

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

malaperle added a comment. In https://reviews.llvm.org/D39050#948500, @akyrtzi wrote: > @malaperle, to clarify we are not suggesting that you write your own parser, > the suggestion is to use clang in 'fast-scan' mode to get the structure of > the declarations of a single file, see

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

nathawes updated this revision to Diff 126065. nathawes added a comment. Herald added a subscriber: mgrang. Worked through the comments from @ioeric and split the code for writing out the collected indexing data into a separate patch. https://reviews.llvm.org/D39050 Files:

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

nathawes added inline comments. Comment at: lib/FrontendTool/ExecuteCompilerInvocation.cpp:170 +Act = index::createIndexDataRecordingAction(FEOpts, std::move(Act)); +CI.setGenModuleActionWrapper(::createIndexDataRecordingAction); + } ioeric wrote: >

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

akyrtzi added a comment. @malaperle, to clarify we are not suggesting that you write your own parser, the suggestion is to use clang in 'fast-scan' mode to get the structure of the declarations of a single file, see `CXTranslationUnit_SingleFileParse` (along with enabling skipping of bodies).

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

malaperle added inline comments. Comment at: include/indexstore/IndexStoreCXX.h:84 + + std::pair getLineCol() { +unsigned line, col; ioeric wrote: > nathawes wrote: > > malaperle wrote: > > > From what I understand, this returns the

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

malaperle added a comment. Hi! I got a bit further in my experiment in integrating this in Clangd. I put some comments (in the first more complete revision). But since the scope of this patch changed, if you feel like we should take the discussions elsewhere, please let me know! Thanks!

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

ioeric added a comment. >> 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

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

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

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

ioeric added a comment. In https://reviews.llvm.org/D39050#922597, @akyrtzi wrote: > 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

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

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

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

ioeric added a comment. >> - 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

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

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.

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

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

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

malaperle added inline comments. Comment at: include/indexstore/IndexStoreCXX.h:75 + bool foreachRelation(llvm::function_ref receiver) { +#if INDEXSTORE_HAS_BLOCKS +return indexstore_occurrence_relations_apply(obj,

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

malaperle added inline comments. Comment at: lib/Index/IndexRecordWriter.cpp:155 +if (!IsNew) { + llvm::errs() << "Index: Duplicate USR! " << SymInfo.USR << "\n"; + // FIXME: print more information so it's easier to find the declaration. I'm

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

malaperle added inline comments. Comment at: lib/Index/IndexingAction.cpp:562 + + SourceManager = CI.getSourceManager(); + DiagnosticsEngine = CI.getDiagnostics(); As a first attempt, I tried to use index::createIndexDataRecordingAction in combination with

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

malaperle added inline comments. Comment at: lib/Index/IndexUnitWriter.cpp:212 + return true; +}; + extra semi-colon (noticed this warning while compiling) https://reviews.llvm.org/D39050 ___ cfe-commits mailing

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

nathawes added inline comments. Comment at: lib/Index/IndexRecordHasher.cpp:103 + COMBINE_HASH(Hasher.hash(param->getType())); +} +return Hash; arphaman wrote: > Should you hash the return type as well? The return type doesn't affect the function's

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

nathawes planned changes to this revision. nathawes added a comment. Thanks @arphaman! I'll work through your comments and update. Comment at: include/clang/Index/IndexDataStoreSymbolUtils.h:13 + +#include "indexstore/indexstore.h" +#include "clang/Index/IndexSymbol.h"

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

arphaman added inline comments. Comment at: include/clang/Index/IndexDataStoreSymbolUtils.h:13 + +#include "indexstore/indexstore.h" +#include "clang/Index/IndexSymbol.h" It looks to me like this header, `"indexstore/indexstore.h"`, and

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

arphaman added a comment. 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