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

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Thanks! Still lg, but more suggestions:



================
Comment at: lld/MachO/Driver.cpp:1146
       parseOrderFile(orderFile);
 
     // FIXME: This prints symbols that are undefined both in input files and
----------------
Could we keep this here so all the treatUndefinedSymbol()s are in one place? Or is there something that forces this to move around?

```
   if (config->outputType == MH_EXECUTE && isa<Undefined>(config->entry)) {
      treatUndefinedSymbol(cast<Undefined(config->entry), "the entry point");
      if (isa<Undefined>(config->entry))
        return false;
    }
```


================
Comment at: lld/MachO/SymbolTable.cpp:190
+void lld::macho::treatUndefinedSymbol(const Undefined &sym, StringRef source) {
+  auto message = [&](const Undefined &sym) {
+    std::string fileName;
----------------
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.)


================
Comment at: lld/MachO/SymbolTable.cpp:198
+            source)
+        .str();
   };
----------------
nit: this reads imho easier like so:

```
std::string message = "undefined symbol: " + toString(sym);
if (!source.empty())
  message +=  "\n>>> referenced by " + source.str();
else
  message +=  "\n>>> referenced by " + toString(sym.getFile());
return message;
```

That was `source` doesn't point to different things based on control flow, and it's fairly linear and straight-forward. (The "referenced by" string is duplicated, but right on top of each other, so it's obvious it's the same)

(It allocates more, but only in a code path that emits diags, so it doesn't normally run anyways.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102143