[PATCH] D54999: [clangd] Populate include graph during static indexing action.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clangd/index/IndexAction.cpp:45
+    auto &Node = I->getValue();
+    if (auto Digest = digestFile(SM, FileID))
+      Node.Digest = std::move(*Digest);
----------------
ilya-biryukov wrote:
> What happens if we can't compute a digest for a file? Are we left with an uninitialized memory?
Yes, we'll be left with some random digest in the node.

But I believe it is as good as any sentinel value, this field is used for consistency checks. Absence of digest implies we will always(well almost always) have an inconsistent(~stale) version of the file in the current structure and will re-trigger indexing action next time. 

Therefore, having this digest initialized to some sentinel or random gibberish has the same probability of colliding with the actual hash. So I believe it is OK to leave it like that.


================
Comment at: clangd/index/IndexAction.cpp:109
     CI.getPreprocessor().addCommentHandler(PragmaHandler.get());
+    if (IncludeGraphCallback != nullptr)
+      CI.getPreprocessor().addPPCallbacks(
----------------
ilya-biryukov wrote:
> NIT: maybe remove `!= nullptr`? the callback is a function, not a pointer, so `nullptr` might be a bit confusing
yeah but this was the convention in the file, is it ok if I change the other usages as well?


================
Comment at: unittests/clangd/IndexActionTests.cpp:126
+}
+
+} // namespace
----------------
ilya-biryukov wrote:
> 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)
> 
I've added a self-include test with header guards, I don't think it is possible to do that without a guard. Wouldn't it cause an infinite loop? I end up getting abort with:
`/clangd-test/header.h:1:10: error: #include nested too deeply`
Did you mean something else?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54999/new/

https://reviews.llvm.org/D54999