[PATCH] D84209: [llvm-libtool-darwin] Add support for -D and -U option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/deterministic-library.test:1-2
+## This test checks that timestamps/etc. are set to 0 by default or when -D
+## option is specified. Timestamps/etc. are preserved when -U option is
+## specified.
----------------
Change "timestamps/etc." to simply "timestamps". You're next paragraph explains the bit about other fields, so the "etc" is somewhat misleading/confusing.

Also, combine these into one sentence and add a couple of missing "the"s: "This test checks that timestamps are set to 0 by default or when the -D option is specified, and that they are preserved when the -U option is specified."


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:25
 
+static bool Deterministic; // Updated by 'D' and 'U' modifiers
+
----------------
I'm not particularly a fan of adding a global variable for this. Are there going to be other configuration options? If so, I'd recommend creating a Config class/struct, that can be created and passed around instead.

(Also, missing trailing full stop in comment)


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:182
 
+static Error checkCommandLineOptions() {
+  if (DeterministicOption && NonDeterministicOption)
----------------
This function does more than just check the options - it also sets behaviour, based on options that are parsed. I think you need to rename the function to clarify. If you add a Config class, perhaps it could become: `static Expected<Config> parseCommandline()` instead, and also do the `cl::ParseCommandLineOptions` call. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84209