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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Writer.cpp:1705
+        if (File && AllNeededIsKnown.count(File))
+          error(File->getName() + ": undefined reference to " + toString(*Sym));
+      }
----------------
MaskRay wrote:
> 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.
I think the current style is fine. Probably the following style is more common in lld though:

  for (...)
    if (...)
      if (auto *F = dyn_cast_or_null<SharedFile<ELFT>>(Sym->File))
        if (F->AllNeededIsKnown)
          error(...)



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D57385