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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov 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);
----------------
kadircet wrote:
> 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.
Reading those values is undefined behavior, which is not the same as reading a random digest.
But anyway, the best way to avoid UB is to make the default constructor initialize it to some value, which can be done independently of this patch.


================
Comment at: clangd/index/IndexAction.cpp:109
     CI.getPreprocessor().addCommentHandler(PragmaHandler.get());
+    if (IncludeGraphCallback != nullptr)
+      CI.getPreprocessor().addPPCallbacks(
----------------
kadircet wrote:
> 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?
Keeping it for consistency LG. Not sure changing the whole file is worth the trouble


================
Comment at: unittests/clangd/IndexActionTests.cpp:126
+}
+
+} // namespace
----------------
kadircet wrote:
> 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?
Yeah, we'll need some cut-off, in might be in a form that's different from the include guard to test this.
The idea was to test a different behavior in the compiler, I believe it will optimize away include-guarded files and call FileSkipped, while it would actually visit the same file twice when it cannot detect an include guard. Something like this should trigger this behavior:

```
#ifndef FOO
#define FOO "main.cpp"
#else
#define FOO "header.h"
#endif

#include FOO
```
Granted, the test-case is obscure, but there's so much C++ code out there, that someone will eventually run clangd on the code doing something like this.

To be clear, I don't think testing anything other than clangd does not crash on this is useful, not saying we should think too much about those cases :-) 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999