Page MenuHomePhabricator

Add index-while-building support to Clang - Part 2
AbandonedPublic

Authored by jkorous on Dec 7 2017, 4:13 PM.

Details

Summary

See Part 1 (https://reviews.llvm.org/D39050) for an overview of the new functionality.

That part added a new -index-store-path option that added a new IndexingAction/WrappingIndexAction implementation to collect file inclusion and unit dependency information in addition to decl occurrences. This part contains the changes to write the collected information out to the directory supplied by the -index-store-path option in the LLVM Bitstream format.

Diff Detail

Event Timeline

nathawes created this revision.Dec 7 2017, 4:13 PM
nathawes planned changes to this revision.Dec 7 2017, 4:27 PM
nathawes retitled this revision from Add index-while-building support to Clang to Add index-while-building support to Clang - Part 2.
nathawes edited the summary of this revision. (Show Details)
nathawes edited the summary of this revision. (Show Details)Dec 19 2017, 10:56 AM
nathawes updated this revision to Diff 127573.Dec 19 2017, 10:59 AM

Updated to account for the most recent changes to Part 1

nathawes planned changes to this revision.Dec 19 2017, 10:59 AM
yvvan added a subscriber: yvvan.Jun 20 2018, 7:23 AM

Can you probably update part 2 and 3 so they can be applied on top of the first patch?

I want to give it a try!

nathawes updated this revision to Diff 153192.Jun 27 2018, 3:24 PM

Updated to apply on the updated first patch.

nathawes planned changes to this revision.Jun 27 2018, 3:25 PM

@yvvan I've just updated them all – sorry for the delay!

nathawes requested review of this revision.Jul 10 2018, 11:02 AM
nathawes edited the summary of this revision. (Show Details)
nathawes added reviewers: ioeric, malaperle, yvvan.
ioeric added inline comments.Aug 3 2018, 4:20 AM
include/clang/Index/IndexDataStoreSymbolUtils.h
1

Names with "util" are generally considered bad... Maybe just call this IndexStore.h as it seems to be the wrapper of indexstore.h for C++?

include/clang/Index/IndexRecordWriter.h
23

Any reason this couldn't be a clang::Decl?

28

which are synthesized by looking at all the occurrences.

I'm not exactly sure what this means. Could you elaborate a bit? Wouldn't each occurrence have its own role? Why do we need to synthesize?

85

Instead of passing in a mutable Error, consider returning llvm::Expected<Result> with llvm::StringError. With that, the result can also be reduced to a boolean.

101

Please document whether Line and Column are 0-based or 1-based.

include/clang/Index/IndexUnitWriter.h
40

Do we expect to add more fields here? Add FIXME?

48

Please document the class.

include/indexstore/indexstore.h
34

I'm not familiar with the macro tricks anymore. Could you explain why INDEXSTORE_VERSION_STRINGIZE_ is needed? Any reason we can't stringify in the INDEXSTORE_VERSION_STRINGIZE?

52

What are the relationships between these enums and those in clang/Index/IndexSymbol.h and libclang. Do they need to match? If so, we should add instructions on what need to be updated when any of them is changed.

lib/Index/ClangIndexRecordWriter.cpp
23

use try_emplace?

24

nit: no brace around one-liners.

36

You could also use StringRef::copy(Allocator)

55

nit: extraneous newline.

69

why this lambda? it seems that code would be simpler if you inline it.

81

nit: emplace_back

87

why? please add comment.

104

We seem to assume that the passed-in Scratch is empty. Maybe clear before using it?

lib/Index/ClangIndexRecordWriter.h
26

Please document this class. There are already a bunch of different writers in this patch...

35

Please document what Hasher is used for.

44

Please document the behavior.

lib/Index/IndexDataStoreUtils.h
1

Same for this file about "util"...

This file is a bit confusing with IndexDataStoreSymbolUtils.h. Are they related? Can they be merged?

lib/Index/IndexRecordHasher.h
31

Please document.

39

These can also use some comments.

lib/Index/IndexingAction.cpp
19

remove the empty lines and let clang-format handle all #includes for you.

lib/Index/UnitIndexDataRecorder.h
41

Instead of having a separate initialization method, could we add a factory method (e.g. create()) that returns llvm::Expected<std::unique_ptr<UnitIndexDataRecorder>>?

jkorous commandeered this revision.Mar 6 2019, 10:45 AM
jkorous added a reviewer: nathawes.
jkorous abandoned this revision.Mar 6 2019, 10:46 AM

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
http://lists.llvm.org/pipermail/cfe-dev/2019-February/061432.html

I'll address comments for this patch in the new set of patches and I'll add original reviewers.