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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
