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