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

[Zhihao Yuan via Phabricator via llvm-commits]

lichray added a comment.

In D83530#2181366 <https://reviews.llvm.org/D83530#2181366>, @jhenderson wrote:

> I think your minimal refactoring bar is perhaps different to mine - I would argue that a refactor that makes the code more complicated is not a good idea.
>
> I'm not arguing against moving to use `OptTable` in general, but I think every single one of my examples there already has at least one, and most more than one, other place that has the same or closely related requirement. We should improve the library first before trying to refactor this code to use it.

I see some complicated code this time and made a few suggestions.  However, to me, duplicated functionality does not imply complicated code.  "Complicated" means that the code itself is difficult to comprehend, with or without some other place that has similar pieces.  I'm fine with duplicating logic that preexists elsewhere during refactoring because we often need to look at more than one copies of the work to figure out a proper abstraction.



================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:227
+  unsigned MAI, MAC;
+  opt::InputArgList Args = Tbl.ParseArgs(makeArrayRef(NewArgv), MAI, MAC);
+  if (Args.hasArg(OPT_help)) {
----------------
Uses of `NewArgv` finish here.  Please consider moving these steps into a function.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:232
+  }
+  {
+    bool HasError = false;
----------------
Please consider moving this into a function with a name that suggests the intent.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:275
+  if (Args.hasArg(OPT_functions)) {
+    Opts.PrintFunctions = FunctionNameKind::LinkageName;
+  } else if (const opt::Arg *A = Args.getLastArg(OPT_functions_EQ)) {
----------------
For example, you may move these into a function called "decideHowToPrintFunctions," then you don't need to immediately show how to do that in code.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:285
+  }
+  if (auto *A = Args.getLastArg(OPT_print_source_context_lines_EQ)) {
+    StringRef V(A->getValue());
----------------
Line 250 seems to have similar logic.  Please consider moving the functionality into a function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83530