[PATCH] D93263: [lld-macho] Implement option: -undefined TREATMENT

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Config.h:33-39
+enum class UndefinedSymbolTreatment : unsigned {
+  unknown = 0,
+  error = 1,
+  warning = 2,
+  suppress = 3,
+  dynamic_lookup = 4,
+};
----------------
it doesn't look like we're actually using the numeric values, so we don't need to assign them. (`: unsigned` is likewise unnecessary)


================
Comment at: lld/MachO/Driver.cpp:609-610
+          .Default(UndefinedSymbolTreatment::unknown);
+  if (config->undefinedSymbolTreatment == UndefinedSymbolTreatment::unknown)
+    error(Twine("malformed undefined-symbol treatment: ") + treatmentStr);
+}
----------------
I think it would be nice set the treatment to `::error` if the option argument is invalid. Then we won't need to handle the `unknown` case later on


================
Comment at: lld/MachO/SymbolTable.cpp:145-146
 
+bool lld::macho::undefinedSymbol(const std::string symbolName,
+                                 const std::string referenceName) {
+  std::string message = "undefined symbol " + symbolName;
----------------
can these be StringRefs?


================
Comment at: lld/MachO/SymbolTable.cpp:163
+  default:
+    fatal("malformed -undefined treatment");
+    return false;
----------------
with the change suggested above, this could be `llvm_unreachable` instead of `fatal`


================
Comment at: lld/MachO/SymbolTable.h:58
 
+extern bool undefinedSymbol(const std::string symbolName,
+                            const std::string referenceName);
----------------
I think this function is a verb-y function but the current name is noun-ish. How about naming it `treatUndefinedSymbol`? (ELF/COFF use `reportUndefinedSymbol`, but since we're not always going to report it...)


================
Comment at: lld/MachO/Writer.cpp:414
         if (isa<Undefined>(s))
-          error("undefined symbol " + toString(*s) + ", referenced from " +
-                toString(isec->file));
+          undefinedSymbol(toString(*s), toString(isec->file));
         else
----------------
doesn't seem like `undefinedSymbol`'s return value is used anywhere. Can it just be a `void` function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93263