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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

HelpHidden assuages my primary user-facing concern about the help text :)

There's a bunch of options we're in all probability going to *never* support, e.g. the Obj-C GC ones, since Obj-C GC is completely unsupported now. I don't know if it's better to remove them entirely or have a different category of "options that LLD will never support", but we can figure that out later.



================
Comment at: lld/MachO/Driver.cpp:250
+  result = 0;
+  SmallVector<StringRef, capacity + 1> parts;
+  str.split(parts, '.', capacity, true);
----------------
gkm wrote:
> 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.
Makes sense, although to be clear, the template argument for a SmallVector is the number of elements that can be inlined. The SmallVector can still grow beyond that; its storage will just need to be heap allocated.


================
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());
----------------
gkm wrote:
> 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"?
That's technically true, but I also feel a bit uncomfortable speaking on behalf of another company :/


================
Comment at: lld/MachO/Driver.cpp:319
+  if (args.hasArg(OPT_help_hidden)) {
+    parser.printHelp(argsArr[0], true);
+    return true;
----------------
Nit: indicate the name of the boolean parameter as a comment, as in `/*helpHidden=*/true`


================
Comment at: lld/MachO/Driver.cpp:322
+  } else if (args.hasArg(OPT_help)) {
+    parser.printHelp(argsArr[0], false);
+    return true;
----------------
Same here.


================
Comment at: lld/MachO/Options.td:9
+// dated 2018-03-07 and cross checked with ld64 source code version
+// 512.4 dated 2018-03-18
 
----------------
You should double check this date ... I'm pretty sure 512.4 is more recent than that.


================
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
----------------
gkm wrote:
> 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?
I misunderstood what you were testing here. This makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80582