ilya-biryukov 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>"},
----------------
ppluzhnikov wrote:
> 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.
Use raw arrays.
================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:546
+ {"include/wordexp.h", "<wordexp.h>"},
+ {"include/x86intrin.h", "<x86intrin.h>"},
+ {"include/xlocale.h", "<cstring>"},
----------------
ppluzhnikov wrote:
> 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.
>
This refactoring makes sense, but please split this into a separate change.
================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:670
+ }};
+ auto *HeaderMapping = new llvm::StringMap<llvm::StringRef>(Mappings->size());
+
----------------
ppluzhnikov wrote:
> 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();
>
There is no guarantee someone won't run this again by mistake in the future
revisions.
Make this function leak-free, don't rely on global invariants.
It's easy to do with a lambda trick from the next comment.
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
