[PATCH] D44236: [llvm-objcopy] Switch over to using TableGen for parsing arguments

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added a comment.

great, many thanks,
I've added a few comments (some minor nits (like names of the variables + formatting)), if you don't mind - let's clang-format -style=llvm these files and update those names,
otherwise this looks good to me.



================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:183
 // system. The only priority is that keeps/copies overrule removes.
-void HandleArgs(Object &Obj, const Reader &Reader) {
+void HandleArgs(const CopyConfig& Config, Object &Obj, const Reader &Reader,
+                               ElfType OutputElfType) {
----------------
nit: const CopyConfig &Config


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:336
 
+void ExecuteElfObjcopy(const CopyConfig& Config) {
+  ElfType OutputElfType;
----------------
const CopyConfig &Config


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:350
+CopyConfig ParseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
+  
+  ObjcopyOptTable T;
----------------
nit: this blank line is probably unnecessary, maybe let's clang-format -style=llvm this diff or the entire file (if not yet).


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:353
+  unsigned MissingArgumentIndex, MissingArgumentCount;
+  auto InputArgs = T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
+
----------------
nit: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
I would use the explicit type InputArgList instead of auto because unlike some other case (i.e. auto *VarDecl = dyn_cast<VarDecl>(Decl)) here it's not mentioned anywhere else. (so, probably, being explicit would improve readability).


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:368
+
+  if (Positional.size() == 0)
+    error("No input file specified");
----------------
very minor nit: Positional.empty() (here and above where we check if size() equals 0)


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:374
+
+  CopyConfig out;
+  out.InputFilename = Positional[0];
----------------
out -> Out,
but, probably, Config would be a better name


Repository:
  rL LLVM

https://reviews.llvm.org/D44236