[PATCH] D121733: Clean pathnames in FileManager.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.

Please allow some time for the clangd team to review this before landing.
Changes to filenames used to cause unintended consequences for us before. We switched to using file UIDs since then, but we're still not sure it's not going to break us.

Also see other comments, there are a few important issues.



================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:24
+  static auto *Mappings =
+      new std::array<std::pair<std::string, std::string>, 645>{{
+          {"algorithm", "<algorithm>"},
----------------
Don't specify sizes of arrays explicitly. This is error prone.


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:546
+          {"include/wordexp.h", "<wordexp.h>"},
+          {"include/x86intrin.h", "<x86intrin.h>"},
+          {"include/xlocale.h", "<cstring>"},
----------------
Why do we change the order of elements here?
Please revert, this increases the diff and is not relevant to the actual change.


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:670
+      }};
+  auto *HeaderMapping = new llvm::StringMap<llvm::StringRef>(Mappings->size());
+
----------------
This line introduces a memory leak.
Notice how the previous version had a `static` variable.


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:677
+  };
+  for (auto &p : *Mappings) {
+    Canonicalize(p.first);
----------------
This function is on a critical path. We don't want to pay for `Canonicalize` on every call to it.
Please create a static variable and initialize in a lambda if that's absolutely necessary.
```
static auto *Mapping = []{ StringMap *Mapping = new ...; /* init code*/ return Mapping; }();
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121733