[PATCH] D53913: [llvm-objcopy] Support --{enable, disable}-deterministic-archives

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:348-349
+      InputArgs.hasArg(OBJCOPY_disable_deterministic_archives))
+    error("Specifying both --enable-deterministic-archives and "
+          "--disable-deterministic-archives is ambiguous");
+  if (InputArgs.hasArg(OBJCOPY_enable_deterministic_archives))
----------------
jhenderson wrote:
> rupprecht wrote:
> > jhenderson wrote:
> > > Is this what GNU objcopy does? I'd actually expect this to be a "last one wins" scenario (similar to various --[no-]some-option in LLD etc.
> > I thought it was simpler to just error instead of match objcopy, but it actually comes to fewer lines implementing last-one-wins. Done.
> You don't need to use the filtered and loop method at all, you can just use the `hasFlag` method, which takes both a positive and negative option:
> 
> I think this is right (various LLD options are implemented this way):
> ```
> Config->DeterministicArchives(OOBJCOPY_enable_deterministic_archvies, OBJCOPY_disable_deterministic_archives, /*default*/true);
> ```
Oh, I didn't see that method. Even shorter now, thanks! :)


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:42
+               "for UIDs, GIDs, and timestamps).">;
+def D : Flag<[ "-" ], "D">, Alias<enable_deterministic_archives>;
+
----------------
jhenderson wrote:
> Somebody pointed out in an llvm-objdump review (see D53804) that Alias options are not in the help text by default (they are hidden), but they really should be. This might want to be a wider issue for us to fix at some point though, but in the meantime, perhaps you should add these particular ones? What do you think?
Agreed that --help is terrible. It also bothers me that we print help for both "--foo=value" and "--foo value". Fixed for these ones.


Repository:
  rL LLVM

https://reviews.llvm.org/D53913