[PATCH] D121733: Clean pathnames in FileManager.

[Paul Pluzhnikov via cfe-commits]

Please note that this patch still breaks Winx64 tests,
and that I marked it as "further changes required" / not ready for review
some time ago.

On Wed, May 11, 2022 at 10:37 AM Ilya Biryukov via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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.
>

std::array requires size.
I could use std::vector instead, at the cost of an extra memory 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>"},
> ----------------
> Why do we change the order of elements here?
>

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.



> 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.
>

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);
> ----------------
> This function is on a critical path.


This function is only called once.



> 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
>
>

-- 
Paul Pluzhnikov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220511/d28c2066/attachment-0001.html>