[PATCH] D57385: [ELF] Support --{,no-}allow-shlib-undefined

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 4 inline comments as done.
MaskRay added inline comments.


================
Comment at: ELF/Writer.cpp:1705
+        if (File && AllNeededIsKnown.count(File))
+          error(File->getName() + ": undefined reference to " + toString(*Sym));
+      }
----------------
grimar wrote:
> It would not change anything, but probably be slightly more consistent to use `toString(File)` here:
> `error(toString(Sym->File) + ": undefined reference to " + toString(*Sym));`
> 
> Actually, I would rewrite this block with the use of early returns, it feels a bit better readable to me, but that is up to you:
> 
> ```
>     for (Symbol *Sym : Symtab->getSymbols()) {
>       if (!Sym->isUndefined() || Sym->isWeak())
>         continue;
>       if (!Sym->File || !isa<SharedFile<ELFT>>(Sym->File))
>         continue;
>       if (AllNeededIsKnown.count(Sym->File))
>         error(toString(Sym->File) + ": undefined reference to " + toString(*Sym));
>     }
> ```
Changed `File->getName()` to `toString(F)`.

As of the early-return pattern, the if condition used here is simple.. So I'd prefer keeping the current form.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57385/new/

https://reviews.llvm.org/D57385