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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Headers.cpp:60
     Argv.push_back(S.c_str());
   IgnoringDiagConsumer IgnoreDiags;
   auto CI = clang::createInvocationFromCommandLine(
----------------
Not related to this patch, but just noticed that we don't call `FS->setCurrentWorkingDirectory(CompileCommand.Directory)` here.
This might break if `CompileCommand` has non-absolute paths.


================
Comment at: clangd/index/CanonicalIncludes.cpp:21
                                    llvm::StringRef CanonicalPath) {
   addRegexMapping((llvm::Twine("^") + llvm::Regex::escape(Path) + "$").str(),
                   CanonicalPath);
----------------
Technically, if one thread initializes `CanonicalIncludes` and the other thread reads from it, this is a data race.
Maybe we're ok with our current usage, but I don't have enough confidence in that (i.e. don't know C++'s memory model good enough) and would advice to take the lock in other functions of `CanonicalIncludes` too.



================
Comment at: clangd/index/FileIndex.cpp:18
 
+const CanonicalIncludes *CanonicalIncludesForSystemHeaders() {
+  static const auto *Includes = [] {
----------------
NIT: `lowerCase` the function name.


================
Comment at: clangd/index/FileIndex.cpp:38
+  const auto *Includes = CanonicalIncludesForSystemHeaders();
+  CollectorOpts.Includes = Includes;
+
----------------
NIT: remove local var, replace with `CollectorOpts.Includes = CanonicalIncludesForSystemHeaders()`?


================
Comment at: unittests/clangd/FileIndexTests.cpp:173
 
+#ifndef LLVM_ON_WIN32
+TEST(FileIndexTest, CanonicalizeSystemHeader) {
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > Why not on Windows?
> because our system header map doesn't really support windows...
Thanks for clarification.
I thought it's standard-library specific, not OS-specific. I would have expected it to work on Windows with libstdc++. (which we're "mocking" here).
Anyway, this not related to this patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462