[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/index/SymbolCollector.cpp:157
+      return Canonical.str();
+    else if (Canonical != Filename)
+      return toURI(SM, Canonical, Opts);
----------------
nit: no need for `else`?


================
Comment at: unittests/clangd/FileIndexTests.cpp:214
 
 TEST(FileIndexTest, HasSystemHeaderMappingsInPreamble) {
+  TestTU TU;
----------------
sammccall wrote:
> I suspect we can replace most of these tests with TestTU - here I just changed the ones where the include guard would otherwise be needed.
This looks good to me.

I think this test case tried to explicitly test that preamble callback has the expected `CanonicalIncludes`. Using `TestTU` seems to achieve the same goal but makes the intention a bit less clear though. 


================
Comment at: unittests/clangd/TestTU.h:55
 
+  // Simulate a header guard of the header (using an #import directive).
+  bool ImplicitHeaderGuard = true;
----------------
is this used?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60316