[PATCH] D42475: [ELF] Add warnings for various symbols that cannot be ordered

[James Henderson via llvm-commits]

On 8 February 2018 at 02:18, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> James Henderson via Phabricator <reviews at reviews.llvm.org> writes:
>
> > +# Check that LLD does not warn for discarded COMDAT symbols, if at
> least instance was kept.
>
> you probably mean "if it is present in the instance that was kept"
>
> > Index: ELF/Driver.cpp
> > ===================================================================
> > --- ELF/Driver.cpp
> > +++ ELF/Driver.cpp
> > @@ -587,6 +587,26 @@
> >    return V;
> >  }
> >
> > +// Parse the symbol ordering file and warn for any duplicate entries.
> > +static std::vector<StringRef>
> > +getSymbolOrderingFile(StringRef Filename) {
>
> This fits in one line, please clang-format.
>
>
> > +  Optional<MemoryBufferRef> MB = readFile(Filename);
> > +  if (!MB)
> > +    return {};
> > +
> > +  std::vector<StringRef> Ret;
> > +  DenseSet<StringRef> Names;
> > +  for (StringRef S : args::getLines(*MB)) {
> > +    bool Inserted = Names.insert(S).second;
> > +    if (Inserted)
> > +      Ret.push_back(S);
> > +    else if (Config->WarnSymbolOrdering)
> > +      warn("symbol ordering file: symbol '" + S + "' specified multiple
> times");
>
> I would remove the Inserted variable.
>
> >    if (auto *Arg = Args.getLastArg(OPT_symbol_ordering_file))
> > -    if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
> > -      Config->SymbolOrderingFile = args::getLines(*Buffer);
> > +    Config->SymbolOrderingFile = getSymbolOrderingFile(Arg->
> getValue());
>
> Why do you need to change this? It is a bit less code to handle the
> Optional in the caller: have getSymbolOrderingFile take a
> MemoryBufferRef.
>

I'm changing this because Rui requested that the parsing and warning for
duplicate entries be in the Driver (see
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180129/521612.html).
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.


>
> LGTM with that.
>
> Thanks,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180208/ba1d7631/attachment.html>