[PATCH] D83530: [llvm-symbolizer] Switch command line parsing from llvm::cl to OptTable

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Honestly, I'm not convinced by the change, looking at this patch, at least not without much more stuff being added to common library code. In particular, I don't like how significant code needs adding in this patch to handle a number of things that were previously handled by the CommandLine code, for example help handling, type validation, and unknown argument detection.

In high-level terms, I don't think I really care either way which command line processing option is used, if it is straightforward to use, so I'm not opposed to a switchover, but at the moment it seems to be significantly complicating things, not simplifying, for relatively little gain.

(P.S. Don't forget to update documentation).



================
Comment at: llvm/test/tools/llvm-symbolizer/untag-addresses.test:5
 # RUN: echo DATA %t.o 0 | llvm-symbolizer | FileCheck --check-prefix=UNTAG %s
-# RUN: echo DATA %t.o 0 | llvm-symbolizer -untag-addresses=0 | FileCheck --check-prefix=NOUNTAG %s
+# RUN: echo DATA %t.o 0 | llvm-symbolizer --no-untag-addresses | FileCheck --check-prefix=NOUNTAG %s
 # RUN: echo DATA %t.o 0 | llvm-addr2line | FileCheck --check-prefix=NOUNTAG %s
----------------
This isn't mentioned in the description, but probably should be.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:78-79
 
-static cl::opt<std::string>
-    ClFallbackDebugPath("fallback-debug-path", cl::init(""),
-                        cl::desc("Fallback path for debug binaries."));
-
-static cl::list<std::string>
-    ClDebugFileDirectory("debug-file-directory", cl::ZeroOrMore,
-                         cl::value_desc("dir"),
-                         cl::desc("Path to directory where to look for debug "
-                                  "files."));
-
-static cl::opt<DIPrinter::OutputStyle>
-    ClOutputStyle("output-style", cl::init(DIPrinter::OutputStyle::LLVM),
-                  cl::desc("Specify print style"),
-                  cl::values(clEnumValN(DIPrinter::OutputStyle::LLVM, "LLVM",
-                                        "LLVM default style"),
-                             clEnumValN(DIPrinter::OutputStyle::GNU, "GNU",
-                                        "GNU addr2line style")));
-
-static cl::opt<bool>
-    ClUseNativePDBReader("use-native-pdb-reader", cl::init(0),
-                         cl::desc("Use native PDB functionality"));
-
 static cl::extrahelp
     HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
 
----------------
Is this redundant now? You've added code that looks like it prints it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83530