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

[Rafael Avila de Espindola via llvm-commits]

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.

LGTM with that.

Thanks,
Rafael