ilya-biryukov added inline comments.
================
Comment at: clangd/index/IndexAction.cpp:23
+// Collects the nodes and edges of include graph during indexing action.
+struct IncludeGraphCollector : public PPCallbacks {
+public:
----------------
Could we add an assertion that the final graph does not contain uninitialized
nodes?
================
Comment at: clangd/index/IndexAction.cpp:28
+
+ // Populates the node for a new include.
+ void FileChanged(SourceLocation Loc, FileChangeReason Reason,
----------------
Maybe expand a bit on what a node is? I.e. mention file digests, URIs, etc.
================
Comment at: clangd/index/IndexAction.cpp:33
+ // We only need to process each file once. So we don't care about anything
+ // but enteries.
+ if (Reason != FileChangeReason::EnterFile)
----------------
s/enteries/entries
================
Comment at: clangd/index/IndexAction.cpp:44
+
+ auto &Node = I->getValue();
+ if (auto Digest = digestFile(SM, FileID))
----------------
Is there a way to check the node has already been populated? Can we do this and
avoid recomputing the hashes for the same file/assert they're the same in
assertion mode?
E.g. by looking at `URI.empty()`?
================
Comment at: clangd/index/IndexAction.cpp:45
+ auto &Node = I->getValue();
+ if (auto Digest = digestFile(SM, FileID))
+ Node.Digest = std::move(*Digest);
----------------
What happens if we can't compute a digest for a file? Are we left with an
uninitialized memory?
================
Comment at: clangd/index/IndexAction.cpp:63
+
+ auto IncludingURI =
+ URIFromFileEntry(SM.getFileEntryForID(SM.getFileID(HashLoc)));
----------------
Maybe convert both URIs before adding anything to `IG`?
To make sure we don't change `IG` unless both URIs can be parsed
================
Comment at: clangd/index/IndexAction.cpp:72
+
+#ifndef NDEBUG
+ // Sanity check to ensure we have already populated a skipped file.
----------------
Maybe move the if block into the function body? To make sure we're be producing
compiler errors on changes to the base function signature regardless of the
compile configuration
================
Comment at: clangd/index/IndexAction.cpp:109
CI.getPreprocessor().addCommentHandler(PragmaHandler.get());
+ if (IncludeGraphCallback != nullptr)
+ CI.getPreprocessor().addPPCallbacks(
----------------
NIT: maybe remove `!= nullptr`? the callback is a function, not a pointer, so
`nullptr` might be a bit confusing
================
Comment at: clangd/index/IndexAction.cpp:133
RefsCallback(Collector->takeRefs());
+ if (IncludeGraphCallback != nullptr)
+ IncludeGraphCallback(std::move(IG));
----------------
NIT: maybe remove `!= nullptr`?
================
Comment at: unittests/clangd/IndexActionTests.cpp:79
+ {testPath("level1_1.h"), "#include \"level2_1.h\""},
+ {testPath("level1_2.h"), "#include \"level2_2.h\""},
+ {testPath("level1_3.h"), "#include \"level2_3.h\""}};
----------------
Why 3 headers on each level? 1 or 2, would probably read a but simpler.
================
Comment at: unittests/clangd/IndexActionTests.cpp:104
+ EXPECT_TRUE(Main.IsTU);
+ EXPECT_EQ(Main.URI.data(), IG.find(PathToURI(MainFilePath))->getKeyData());
+
----------------
Directly looking at the graph to capture the include structure might be more
straightforward, e.g.
```
EXPECT_THAT(graph,
UnorderedElementsAre(Pair(Main, IncludersAre("level1", ....)))
```
This won't work on `StringMap`, but converting to `std::map<string, node>`
should be simple. Other invariants can be tested separately (e.g. values of
`IsTU`)
Maybe try that?
================
Comment at: unittests/clangd/IndexActionTests.cpp:126
+}
+
+} // namespace
----------------
Could we also test a few corner cases?
- Self-includes (with and without include guards).
- Skipped files (multiple includes of the same file with `#pragma once` or
include guards)
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
