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

[James Henderson via llvm-commits]

Wait, never mind - I completely misread your comment.

On 8 February 2018 at 09:35, James Henderson <jh7370.2008 at my.bristol.ac.uk>
wrote:

>
>
> 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->get
>> Value());
>>
>> 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/6c5d720c/attachment.html>