[PATCH] D80582: [lld-macho] Specify the complete set of command-line options for ld64

[Greg McGary via Phabricator via llvm-commits]

gkm marked 9 inline comments as done.
gkm added a subscriber: compnerd.
gkm added inline comments.


================
Comment at: lld/MachO/Driver.cpp:248
+template <typename UI, int capacity, int width>
+static bool parsePackedVersion(StringRef str, UI &result) {
+  result = 0;
----------------
smeenai wrote:
> This should return an `llvm::Expected` instead of a boolean, which would also allow you to have different error messages for the various parse failures.
> 
> My understanding is that the packing is needed because that's how the corresponding load commands are written out. In that case, I'd argue that there's multiple concerns going on in this function. Ideally this would only parse the version out into a tuple, and then the packing would be a separate function in the writer that's called when you're writing out the load command. The other advantage of that would be that if we need to make decisions based on these values, a tuple is a lot easier to process than a packed value. (E.g. ld64 emits either an `LC_BUILD_VERSION` or an `LC_VERSION_MIN_MACOSX` depending on your minimum version.)
I am deferring to @compnerd's D81413 for `-platform-version` improvements , so am removing my changes from this diff.


================
Comment at: lld/MachO/Driver.cpp:250
+  result = 0;
+  SmallVector<StringRef, capacity + 1> parts;
+  str.split(parts, '.', capacity, true);
----------------
smeenai wrote:
> Nit: why the `+ 1`? Is it just to account for potentially erroneous input?
Yes: if there are excess components, this will allow split to detect them.


================
Comment at: lld/MachO/Driver.cpp:294
+    usageError(arg);
+  // FIXME: add semantics
+}
----------------
smeenai wrote:
> I would imagine the semantics would be storing the result in the Config and then outputting the appropriate load command in the writer?
Yes


================
Comment at: lld/MachO/Driver.cpp:327
+  if (opt.getGroup().getID() == OPT_grp_deprecated) {
+    warn("Option `" + opt.getPrefixedName() + "' is deprecated in ld64:");
+    warn(opt.getHelpText());
----------------
smeenai wrote:
> I'm torn about referencing ld64 in these messages. On the one hand, it is the motivation for our behavior. On the other hand, all our users might not know what ld64 is (or at least know it by that name), or they might be confused why LLD is giving error messages that reference ld64.
> 
> I'm leaning towards just making these say "is deprecated" instead of "is deprecated in ld64". What do you think?
How about saying "... is deprecated by Apple"?


================
Comment at: lld/test/MachO/platform-version.test:47
+USAGE: usage: -platform_version <platform> <min_version> <sdk_version>
+USAGE: undefined symbol: _main
+MISSING: -platform_version: missing argument
----------------
smeenai wrote:
> I wouldn't add this check, since it's not the main point of the test, and it won't be produced if we bail out early on input argument errors.
The complaint about undefined `_main` happens after the arg-parsing phase, by expecting it, we are asserting that no unexpected diagnostic appears during arg parsing. 
What do you recommend as an alternative method?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80582