[PATCH] D43462: [clangd] Fixes for #include insertion.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Headers.cpp:45
+bool hasHeaderExtension(PathRef Path) {
+  constexpr static const char *HeaderExtensions[] = {".h", ".hpp", ".hh",
+                                                     ".hxx"};
----------------
There is another list of header extensions in `ClangdServer::switchSourceHeader`, let's reuse the list of extensions used here and there.


================
Comment at: clangd/Headers.cpp:48
+  return std::any_of(std::begin(HeaderExtensions), std::end(HeaderExtensions),
+                     [&Path](const char *Ext) { return Path.endswith(Ext); });
+}
----------------
Maybe use `llvm::sys::path::extension` and search it in `HeaderExtensions` using `StringRef::equals_lower`?
Will also handle upper- and mixed-case.


================
Comment at: clangd/Headers.cpp:60
+                     IntrusiveRefCntPtr<vfs::FileSystem> FS) {
+  if (File == Header || !hasHeaderExtension(Header))
+    return "";
----------------
Maybe replace `!hasHeaderExtension(Header)` to `hasSourceExtension(Header)`?
Knowing about all header extensions in advance is hard, knowing a subset of source extensions seems totally true.


================
Comment at: clangd/index/FileIndex.cpp:18
 
+CanonicalIncludes *CanonicalIncludesForSystemHeaders() {
+  auto *Includes = new CanonicalIncludes();
----------------
Maybe store a static inside the function body (to properly free memory) and return a const reference (to avoid mutating shared data)?

```
const CanonicalIncludes &CanonicalIncludesForSystemHeaders() {
  static CanonicalInclude Includes = []() -> CanonicalHeader {
    CanonicalInclude Includes;
    addSystemHeadersMapping(Includes);
    return Includes;
  };
  return Includes;
}
```

Also Sam mentioned that `CanonicalIncludes` is not thread-safe, as the `Regex`'s `match()` mutates.


================
Comment at: unittests/clangd/FileIndexTests.cpp:173
 
+#ifndef LLVM_ON_WIN32
+TEST(FileIndexTest, CanonicalizeSystemHeader) {
----------------
Why not on Windows?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462