[PATCH] D127562: [lld-macho] Add support for exporting no symbols

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1412
+      emptyExportedSymbolsList || args.hasArg(OPT_no_exported_symbols);
+  if ((!config->exportedSymbols.empty() || exportNoSymbols) &&
+      !config->unexportedSymbols.empty()) {
----------------
hmm should we make it an error to use `-no_exported_symbols` with `-exported_symbol` as well?


================
Comment at: lld/MachO/Driver.cpp:1415-1416
     error("cannot use both -exported_symbol* and -unexported_symbol* options\n"
           ">>> ignoring unexports");
     config->unexportedSymbols.clear();
   }
----------------
tangential & existing code, but this seems kind of redundant to me lol. We're already erroring out, doesn't seem like there's a need to tell the user we're ignoring `unexports`. (Not sure there's a need to clear the unexported symbols list either, but I suppose it might make any subsequent error messages less confusing.)


================
Comment at: lld/MachO/Driver.cpp:1561
 
-    if (!config->exportedSymbols.empty()) {
+    if (!config->exportedSymbols.empty() || exportNoSymbols) {
       parallelForEach(symtab->getSymbols(), [](Symbol *sym) {
----------------
IMO this check is a bit confusing because it conflates a perf optimization with an actual difference in behavior. How about having the Config be something like

```
struct Config {
  ...
  bool explicitExports = false;
  SymbolPatterns exportPatterns; // (can represent both `exportedSymbols` and `unexportedSymbols`)
};
```

With `exportPatterns` interpreted differently based on the value of `explicitExports`. Then this `if` check becomes `if (config->explicitExports) { ...`

That said I know this is existing code, and refactoring it can be rightly said to be out of scope :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127562