<div dir="ltr">Wait, never mind - I completely misread your comment.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On 8 February 2018 at 09:35, James Henderson <span dir="ltr"><<a href="mailto:jh7370.2008@my.bristol.ac.uk" target="_blank">jh7370.2008@my.bristol.ac.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">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="m_4838573631028251590gmail-">James Henderson via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> writes:<br>
<br>
</span><span class="m_4838573631028251590gmail-">> +# 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="m_4838573631028251590gmail-"><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(StringR<wbr>ef Filename) {<br>
<br>
This fits in one line, please clang-format.<br>
<span class="m_4838573631028251590gmail-"><br>
<br>
> +  Optional<MemoryBufferRef> MB = readFile(Filename);<br>
</span>> +  if (!MB)<br>
> +    return {};<br>
<span class="m_4838573631028251590gmail-">> +<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="m_4838573631028251590gmail-"><br>
>    if (auto *Arg = Args.getLastArg(OPT_symbol_ord<wbr>ering_file))<br>
> -    if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))<br>
> -      Config->SymbolOrderingFile = args::getLines(*Buffer);<br>
> +    Config->SymbolOrderingFile = getSymbolOrderingFile(Arg->get<wbr>Value());<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></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" target="_blank">http://lists.llvm.org/<wbr>pipermail/llvm-commits/Week-<wbr>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><span class=""><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></span></div><br></div></div>
</blockquote></div><br></div>