[PATCH] D125658: [ifs] Switch to using OptTable

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: MaskRay.
jhenderson added a subscriber: MaskRay.
jhenderson added a comment.
Herald added a subscriber: StephenFan.

I've added @MaskRay as he's done a number of `cl::opt` -> `OptTable` transitions in other tools.

One thing I haven't done yet is ensure that all old options are still present in the new tool. A task for later (or someone else).



================
Comment at: llvm/tools/llvm-ifs/Opts.td:11
+
+def help : FF<"help", "Display this help">;
+def : F<"h", "Alias for --help">, Alias<help>;
----------------
I'd generally put these options in alphabetical order, to make it easier to find options.


================
Comment at: llvm/tools/llvm-ifs/Opts.td:37
+defm output_tbd : Eq<"output-tbd", "Output path for TBD file">;
+def write_if_changed : FF<"write-if-changed", "Write the output file only if it is new or has changed.">;
----------------
Let's normalise the help text whilst we're here (i.e. no trailing full stop).


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:95-101
+  bool StripIfsArch = false;
+  bool StripIfsBitwidth = false;
+  bool StripIfsEndianness = false;
+  bool StripIfsTarget = false;
+  bool StripUndefined = false;
+  bool StripNeeded = false;
+  bool StripSize = false;
----------------
Aphabetical order within these groups would make sense, I think.


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:311
+    if (!Config.InputFormat)
+      llvm::errs() << "Invalid argument '" << A->getValue() << "'\n";
+  }
----------------
Should this have a `std::exit(1)` after it? Also, I believe you should be using `WithColor::error()` like other error reporting? Same as others below.

Perhaps worth factoring out error reporting into a different function to ensure bits like that aren't missed.


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:322-323
+          << ToolName
+          << ": for the --output-format option: Cannot find option named '"
+          << A->getValue() << "'!";
+      std::exit(1);
----------------
Also applies to other similar cases.


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:331
+  if (const opt::Arg *A = Args.getLastArg(OPT_bitwidth_EQ)) {
+    int width;
+    llvm::StringRef S(A->getValue());
----------------
1) Why is this an `int` rather than something unsigned like `size_t` or `uint64_t` etc?
2) `width` -> `Width` (upper-case for variable names).


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:345-348
+        StringSwitch<Optional<IFSEndiannessType>>(A->getValue())
+            .Case("little", IFSEndiannessType::Little)
+            .Case("big", IFSEndiannessType::Big)
+            .Default(None);
----------------
This needs an error if an invalid option is specified (and a corresponding test case). Otherwise --endianness=foo will be accepted.


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:527-529
+        WithColor::error() << "Couldn't open " << *Config.Output
                            << " for writing.\n";
         return -1;
----------------
Code like this shows more need for a noreturn error function, to save weirdness like inconsistent return values.

May also be worth a separate patch bringing all error messages into line with https://llvm.org/docs/CodingStandards.html#error-and-warning-messages.


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

https://reviews.llvm.org/D125658