[PATCH] D62434: [lld-link] diagnose undefined symbols before LTO when possible

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Generally looking ogod.



================
Comment at: lld/COFF/SymbolTable.cpp:146
+    return getSymbolLocations(b);
+  return {};
+}
----------------
inglorion wrote:
> ruiu wrote:
> > This line is unreachable as you explicitly asserted it by `assert`, no?
> > 
> > I think you can change the second condition to `return getSymbolLocations(cast<BitcodeFile>(file));`
> > This line is unreachable as you explicitly asserted it by assert, no?
> 
> When asserts are enabled, yes. But they aren't always.
> 
> I'm not super concerned that we would ever change this to pass in other types of file and forget to update it, but I kind of like the behavior as it is: if we do that with asserts enabled, we will get an informative message, and if we do it with asserts disabled, just don't return any symbol locations.
> 
> That said, if you want me to change it, I will.
I feel like returning a "reasonable" value for an impossible condition does more harm than mitigating a pain. When a program reaches here under an unintended condition, something bad has already occurred, and we should report it of swallowing it.

How about changing the last line to llvm_unreachable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62434