<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 8 February 2018 at 02:18, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">James Henderson via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> writes:<br>
<br>
</span><span class="gmail-">> +# Check that LLD does not warn for discarded COMDAT symbols, if at least instance was kept.<br>
<br>
</span>you probably mean "if it is present in the instance that was kept"<br>
<span class="gmail-"><br>
> Index: ELF/Driver.cpp<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- ELF/Driver.cpp<br>
> +++ ELF/Driver.cpp<br>
</span>> @@ -587,6 +587,26 @@<br>
>    return V;<br>
>  }<br>
><br>
> +// Parse the symbol ordering file and warn for any duplicate entries.<br>
> +static std::vector<StringRef><br>
> +getSymbolOrderingFile(<wbr>StringRef Filename) {<br>
<br>
This fits in one line, please clang-format.<br>
<span class="gmail-"><br>
<br>
> +  Optional<MemoryBufferRef> MB = readFile(Filename);<br>
</span>> +  if (!MB)<br>
> +    return {};<br>
<span class="gmail-">> +<br>
> +  std::vector<StringRef> Ret;<br>
> +  DenseSet<StringRef> Names;<br>
</span>> +  for (StringRef S : args::getLines(*MB)) {<br>
> +    bool Inserted = Names.insert(S).second;<br>
> +    if (Inserted)<br>
> +      Ret.push_back(S);<br>
> +    else if (Config->WarnSymbolOrdering)<br>
> +      warn("symbol ordering file: symbol '" + S + "' specified multiple times");<br>
<br>
I would remove the Inserted variable.<br>
<span class="gmail-"><br>
>    if (auto *Arg = Args.getLastArg(OPT_symbol_<wbr>ordering_file))<br>
> -    if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))<br>
> -      Config->SymbolOrderingFile = args::getLines(*Buffer);<br>
> +    Config->SymbolOrderingFile = getSymbolOrderingFile(Arg-><wbr>getValue());<br>
<br>
</span>Why do you need to change this? It is a bit less code to handle the<br>
Optional in the caller: have getSymbolOrderingFile take a<br>
MemoryBufferRef.<br></blockquote><div><br></div><div>I'm changing this because Rui requested that the parsing and warning for duplicate entries be in the Driver (see <a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180129/521612.html">http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180129/521612.html</a>). I don't have strong feelings on it either way, but there appears to be a disagreement between what you prefer and what Rui prefers here, so I'd like this agreed before I try to commit anything.<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">
<br>
LGTM with that.<br>
<br>
Thanks,<br>
Rafael<br>
</blockquote></div><br></div></div>