This is an archive of the discontinued LLVM Phabricator instance.

[index-while-building] PathIndexer
ClosedPublic

Authored by jkorous on Aug 27 2019, 6:09 PM.

Details

Summary

Part of i-w-b upstreaming https://reviews.llvm.org/D64384

This is actually a bit polished PathStorage.

Diff Detail

Event Timeline

jkorous created this revision.Aug 27 2019, 6:09 PM

I tried my best to provide meaningful comments, however, I can't find users of PathIndexer in https://reviews.llvm.org/D64384, so I don't think I understand this abstraction at all. I would appreciate some big-picture description.

clang/include/clang/IndexSerialization/PathIndexer.h
20

"namespace clang"?

36

s/Represents// (throughout the patch)

"Represents" is generally a filler word -- of course types represent some concept, they rarely *are* that concept itself, except for concepts that we invent ourselves.

39

Would be best to not refer to one concept with two terms -- root and prefix.

40

"Path relative to the root specified by PrefixKind."

41

Const members in structs are not great because they make structs not act like values -- the assignment operator becomes deleted. (Throughout the patch.)

56

Builds => stores, or maintains. It does not walk the filesystem itself.

66

In this context, "\param".

80

Add an assert for uniqueness?

Rename to storeNewPath?

clang/lib/IndexSerialization/PathIndexer.cpp
15

It is OK to shadow member variables with parameters. Shadowing does the right thing in the member initializer list.

Oh, it is called PathStorage there. Looking.

gribozavr added inline comments.Sep 2 2019, 7:54 AM
clang/include/clang/IndexSerialization/PathIndexer.h
34

I believe it would be helpful to build a separate class that just does string interning, and let BitPathComponent be a reference to a string in that string pool. That would simplify code in PathIndexer, and separate the concerns of path handling and string storage. PathIndexer would expose a reference to the interned storage through a getter.

54

WDYT about moving DirBitPath and FileBitPath to be nested types in PathIndexer? You should be able to also remove "Bit" from their names then.

57

SerializablePaths? SerializablePathCollection? OnDiskPathCollection? (like clang/lib/Serialization/MultiOnDiskHashTable.h)

I'm not a fan of using the word "Indexer" for a passive entity.

76

WDYT about using optional instead of a special marker value?

clang/lib/IndexSerialization/PathIndexer.cpp
22

s/FileBitPaths.size()/FileToIndex.size()/ would probably be more intuitive.

jkorous marked 17 inline comments as done.Sep 20 2019, 11:35 AM
jkorous added inline comments.
clang/include/clang/IndexSerialization/PathIndexer.h
20

clang::index?

41

I don't have a particularly strong opinion but in general I like immutability as an invariant.

76

Good point! Originally the -1 convention for both return values and indices being passed as parameters was used throughout the i-w-b. It avoided explicit error handling which allowed code to look simpler but was more difficult to reason about. This seems to be the last surviving specimen.

jkorous updated this revision to Diff 221084.Sep 20 2019, 11:44 AM
jkorous marked 3 inline comments as done.

Addressed the comments and tried to simplify things by splitting into multiple smaller classes.

@arphaman can you please take a look at this patch?

Sure, will do.

jkorous updated this revision to Diff 279310.Jul 20 2020, 11:02 AM

Restore the patch. Previous update wasn't meant for this review - I confused the review IDs.

arphaman added inline comments.Jul 20 2020, 8:03 PM
clang/include/clang/IndexSerialization/SerializablePathCollection.h
32 ↗(On Diff #279310)

I feel like StringOffSz isn't very informative. Can this be renamed to something like StringPool::OffsetSize, or maybe StringPool::Index.

clang/lib/IndexSerialization/SerializablePathCollection.cpp
48 ↗(On Diff #279310)

Do you think you can use llvm::sys::path::starts_with instead? why is it important to return false when dir is equivalent to rest. Would we be able to store paths a path like /path/to/xyz_ab, when the CWD/Sysroot is /path/to/xyz when we don't have this check?

55 ↗(On Diff #279310)

I looked through the uses of tryStoreFilePath in the full patch, and it doesn't seem like the use of try in the name is justified, since it could imply that some form of error could be produced as a result of a call. Can this be named more precisely, perhaps something like getStoreIndexForFile()? Also, uses seem to imply that it's always converted to uint64_t, can we avoid the conversion and return uint64_t here instead?

68 ↗(On Diff #279310)

Same name critique, could this be named something like storeDirPath, without the try?

83 ↗(On Diff #279310)

Please use is_separator instead of comparing to /

jkorous added inline comments.Aug 4 2020, 2:38 PM
clang/lib/IndexSerialization/SerializablePathCollection.cpp
48 ↗(On Diff #279310)

I'll try to get rid of this. It's only used in tryStoreDirPath(). The logic is probably sound but maybe it could be implemented in a more readable fashion.

55 ↗(On Diff #279310)

The semantics of the name are the same as STL containers - say std::map::try_emplace. I'm open to renaming but would like to have a name that conveys the fact that this is actually the method used for gathering the data for serialization.

68 ↗(On Diff #279310)

Maybe storeUniqueDirPath or storeNewDirPath? Because we aren't always storing the input.

83 ↗(On Diff #279310)

That's a good point!

jkorous updated this revision to Diff 283111.Aug 4 2020, 7:44 PM

Addressed comments.

jkorous marked an inline comment as done.Aug 4 2020, 7:45 PM

Friendly ping

arphaman accepted this revision.Aug 18 2020, 11:38 AM
This revision is now accepted and ready to land.Aug 18 2020, 11:38 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 11:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript