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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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))
----------------
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);
```


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:42
+               "for UIDs, GIDs, and timestamps).">;
+def D : Flag<[ "-" ], "D">, Alias<enable_deterministic_archives>;
+
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D53913