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