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

[Alex Brachet via Phabricator via llvm-commits]

abrachet added a comment.

In D125658#3515046 <https://reviews.llvm.org/D125658#3515046>, @jhenderson wrote:

> 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).

Shouldn't this be covered by tests?



================
Comment at: llvm/tools/llvm-ifs/Opts.td:33
+defm output : Eq<"output", "Output file **DEPRECATED**">;
+def : Joined<["--"], "o">, HelpText<"Alias for --output">, Alias<output_EQ>;
+defm output_elf : Eq<"output-elf", "Output path for ELF file">;
----------------
MaskRay wrote:
> `--o` looks a bit weird. The convention for short options is `-o`. You may need to check whether your downstream uses `--o` and removes the usage.
> 
> It's fine to support both `-` and `--` for now and remove `--` later.
Thanks good catch, that was just my mistake no one is using that.


================
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);
----------------
jhenderson wrote:
> Also applies to other similar cases.
I did it this way to match what llvm::cl errors looked like, and keep the tests the same. WDYT about fixing these errors and others to be more in line with https://llvm.org/docs/CodingStandards.html#error-and-warning-messages in a separate patch?


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:527-529
+        WithColor::error() << "Couldn't open " << *Config.Output
                            << " for writing.\n";
         return -1;
----------------
jhenderson wrote:
> 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.
Indeed, I will more the errors not added by this patch to use the `fatalError` overload I added in a separate patch.


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

https://reviews.llvm.org/D125658