[PATCH] D102143: [lld-macho] Treat undefined symbols uniformly

[Jez Ng via Phabricator via llvm-commits]

int3 marked 3 inline comments as done.
int3 added inline comments.
Herald added a reviewer: gkm.


================
Comment at: lld/MachO/SymbolTable.cpp:190
+void lld::macho::treatUndefinedSymbol(const Undefined &sym, StringRef source) {
+  auto message = [&](const Undefined &sym) {
+    std::string fileName;
----------------
thakis wrote:
> int3 wrote:
> > thakis wrote:
> > > nit: s/&/source/
> > > 
> > > And if we're doing capturing, do `[source, sym]` and remove the arg, to make things self-consistent.
> > > 
> > > (`&` works here, but with `&` it's easy to accidentally capture a stack variable, and if the closure escapes that's a recipe for subtle bugs, and compilers don't yet warn on default-capturing locals in escaping closures. So I try to not use default capture.)
> > fair enough. while at it I renamed `message` to `msg` so it doesn't shadow the `message` defined in `lld/Common`.
> meh, the local `message` string shadows too. i'd keep the old name (but up to you)
fair point...


================
Comment at: lld/MachO/SymbolTable.cpp:194
+      return message + "\n>>> referenced by " + source.str();
+    else
+      return message + "\n>>> referenced by " + toString(sym.getFile());
----------------
thakis wrote:
> Since you're doing `return`s in the conditional now: style guide says no else after return :P
I don't like this part of the style guide :p Guess I'll go with your original suggestion to preserve symmetry


================
Comment at: lld/MachO/Writer.cpp:1061
+      treatUndefinedSymbol(*undefined, "the entry point");
+    if (!isa<Undefined>(config->entry))
+      prepareBranchTarget(config->entry);
----------------
thakis wrote:
> ...this change is no longer needed now that this is done in driver, rigth?
I swore I deleted it... oops


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102143