<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace;color:#000000">Please note that this patch still breaks Winx64 tests,</div><div class="gmail_default" style="font-family:monospace,monospace;color:#000000">and that I marked it as "further changes required" / not ready for review</div><div class="gmail_default" style="font-family:monospace,monospace;color:#000000">some time ago.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 11, 2022 at 10:37 AM Ilya Biryukov via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">ilya-biryukov requested changes to this revision.<br>
ilya-biryukov added a comment.<br>
This revision now requires changes to proceed.<br>
<br>
Please allow some time for the clangd team to review this before landing.<br>
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.<br>
<br>
Also see other comments, there are a few important issues.<br>
<br>
<br>
<br>
================<br>
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:24<br>
+ static auto *Mappings =<br>
+ new std::array<std::pair<std::string, std::string>, 645>{{<br>
+ {"algorithm", "<algorithm>"},<br>
----------------<br>
Don't specify sizes of arrays explicitly. This is error prone.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">std::array requires size.</div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">I could use std::vector instead, at the cost of an extra memory allocation.</div></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
================<br>
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:546<br>
+ {"include/wordexp.h", "<wordexp.h>"},<br>
+ {"include/x86intrin.h", "<x86intrin.h>"},<br>
+ {"include/xlocale.h", "<cstring>"},<br>
----------------<br>
Why do we change the order of elements here?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">Note that the elements are inserted into a map</div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">(after commit b3a991df3cd6a; used to be a vector before).</div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)"><br></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">Also note that there are duplicates, e.g.</div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)"><br></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">{"bits/typesizes.h", "<cstdio>"},<br></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">{"bits/typesizes.h", "<sys/types.h>"},</div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">which can't work as intended / is already broken.</div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)"><br></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">Sorting helps to find these duplicates.</div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Please revert, this increases the diff and is not relevant to the actual change.<br>
<br>
<br>
================<br>
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:670<br>
+ }};<br>
+ auto *HeaderMapping = new llvm::StringMap<llvm::StringRef>(Mappings->size());<br>
+<br>
----------------<br>
This line introduces a memory leak.<br>
Notice how the previous version had a `static` variable.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">No, it does not. This function is called only once to initialize a static variable: </div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)"><br></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)"> static const auto *SystemHeaderMap = GetHeaderMapping();</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
================<br>
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:677<br>
+ };<br>
+ for (auto &p : *Mappings) {<br>
+ Canonicalize(p.first);<br>
----------------<br>
This function is on a critical path.</blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(0,0,0)">This function is only called once.</div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> We don't want to pay for `Canonicalize` on every call to it.<br>
Please create a static variable and initialize in a lambda if that's absolutely necessary.<br>
```<br>
static auto *Mapping = []{ StringMap *Mapping = new ...; /* init code*/ return Mapping; }();<br>
```<br>
<br>
<br>
Repository:<br>
rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
<a href="https://reviews.llvm.org/D121733/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D121733/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D121733" rel="noreferrer" target="_blank">https://reviews.llvm.org/D121733</a><br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature">Paul Pluzhnikov</div></div>