[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via Phabricator via cfe-commits]

ppluzhnikov planned changes to this revision.
ppluzhnikov added inline comments.


================
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>"},
----------------
ilya-biryukov wrote:
> Don't specify sizes of arrays explicitly. This is error prone.
std::array requires size.

I could use std::vector instead, at the cost of an extra allocation.


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:546
+          {"include/wordexp.h", "<wordexp.h>"},
+          {"include/x86intrin.h", "<x86intrin.h>"},
+          {"include/xlocale.h", "<cstring>"},
----------------
ilya-biryukov wrote:
> Why do we change the order of elements here?
> Please revert, this increases the diff and is not relevant to the actual change.
Note that the elements are inserted into a map
(after commit b3a991df3cd6a; used to be a vector before).

Also note that there are duplicates, e.g.

{"bits/typesizes.h", "<cstdio>"},
{"bits/typesizes.h", "<sys/types.h>"},

which can't work as intended / is already broken.

Sorting helps to find these duplicates.



================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:670
+      }};
+  auto *HeaderMapping = new llvm::StringMap<llvm::StringRef>(Mappings->size());
+
----------------
ilya-biryukov wrote:
> This line introduces a memory leak.
> Notice how the previous version had a `static` variable.
No, it does not. This function is called only once to initialize a static variable: 

    static const auto *SystemHeaderMap = GetHeaderMapping();
 


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:677
+  };
+  for (auto &p : *Mappings) {
+    Canonicalize(p.first);
----------------
ilya-biryukov wrote:
> 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; }();
> ```
This function is only called once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121733