ilya-biryukov added inline comments.
================
Comment at: clangd/index/IndexAction.cpp:31
+ // Populates the following fields of the corresponding node for a new
include:
+ // - Digest -> SHA1 hash of the file.
+ // - IsTU -> true if the file is the main file the indexing action has been
----------------
No need to duplicate the comments here, one can always look at the struct
definition.
Mentioning it populates everything except the include structure should be
enough.
================
Comment at: clangd/index/IndexAction.cpp:36
+ // We perform this population in FileChanged rather than InclusionDirective
to
+ // get rid of possible IO, since we have FileID in this callback we can use
it
+ // to query SourceManager and use the cached file if already read.
----------------
No need to mention IO, something like the following should convey the same
message: `we cannot populate the fields in InclusionDirective because it does
not have access to the contents of the target file`
================
Comment at: unittests/clangd/IndexActionTests.cpp:28
+std::string PathToURI(llvm::StringRef Path) {
+ return URI::create(Path).toString();
+}
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Maybe inline this? This looks simple enough.
> yeah but repeating all of that over and over again looks really ugly.
Maybe call it `toUri` to make it even shorther then?
Or at least `pathToURI` to follow the naming conventions.
================
Comment at: unittests/clangd/IndexActionTests.cpp:52
+ const auto &Node = IndexFile.Sources->lookup(URI);
+ EXPECT_EQ(Node.URI.data(), IndexFile.Sources->find(URI)->getKeyData());
+ }
----------------
NIT: add a comment that uninitialized nodes will have an empty URI?
================
Comment at: unittests/clangd/IndexActionTests.cpp:109
+
+ IndexFileIn IndexFile = runIndexingAction(MainFilePath);
+ ASSERT_TRUE(IndexFile.Sources);
----------------
Do we ever use anything but the include graph?
Maybe create a helper function that return `vector<pair<string,
IncludeGraphNode>>` (or even `map<string, IncludeGraphNode>`) to avoid
repeating the conversion code in every test?
================
Comment at: unittests/clangd/IndexActionTests.cpp:128
+
+ checkNodesAreInitialized(
+ IndexFile, {MainFilePath, Level1Header.first, Level2Header.first});
----------------
Could we move these checks into the body of `runIndexingAction` to avoid
repeating them in every test?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54999/new/
https://reviews.llvm.org/D54999
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
