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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 3 inline comments as done.
sameerarora101 added inline comments.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:25
 
+static bool Deterministic; // Updated by 'D' and 'U' modifiers
+
----------------
jhenderson wrote:
> 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)
For now, there aren't any other configuration options that vary depending on the flags passed in on the command line. However, I can still wrap this in a Config class/struct just in case one is needed in the future? What do you think?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:182
 
+static Error checkCommandLineOptions() {
+  if (DeterministicOption && NonDeterministicOption)
----------------
jhenderson wrote:
> 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?
Yup, I agree on the name and `cl::ParseCommandLineOptions` call, I have changed it accordingly now. (I can change the signature too if we decide on adding the Config class - please take a look at my comment above for that). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84209