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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


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


================
Comment at: clangd/index/FileIndex.cpp:18
 
+CanonicalIncludes *CanonicalIncludesForSystemHeaders() {
+  auto *Includes = new CanonicalIncludes();
----------------
ilya-biryukov wrote:
> 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.
As discussed offline, we want to avoid destructing static objects because ... 

Made `CanonicalIncludes` thread-safe by adding a mutex around matches.


================
Comment at: unittests/clangd/FileIndexTests.cpp:173
 
+#ifndef LLVM_ON_WIN32
+TEST(FileIndexTest, CanonicalizeSystemHeader) {
----------------
ilya-biryukov wrote:
> Why not on Windows?
because our system header map doesn't really support windows...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462