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

[Bob Haarman via Phabricator via llvm-commits]

inglorion added inline comments.


================
Comment at: lld/COFF/SymbolTable.cpp:146
+    return getSymbolLocations(b);
+  return {};
+}
----------------
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.


================
Comment at: lld/COFF/SymbolTable.cpp:262
 
-void SymbolTable::reportRemainingUndefines() {
+/// Emit diagnostics for symbols in undefs and localImports.
+static void
----------------
ruiu wrote:
> Can you expand the comment to explain what this function does a little bit more in detail?
Is this better?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62434