[PATCH] D56629: [llvm-readelf] Allow single-letter flags to be merged.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

One question: what happens if you do enable grouping, and there's an ambiguous alias?



================
Comment at: test/tools/llvm-readobj/merged.test:5
+RUN: cmp %t.merged %t.not-merged
+RUN: cat %t.merged | FileCheck %s
+
----------------
Use FileCheck --input-file instead of cat.


================
Comment at: test/tools/llvm-readobj/merged.test:10
+# were supported.
+RUN: not llvm-readobj -aeWhSrnudlVgIs %p/Inputs/trivial.obj.elf-i386 |& FileCheck %s --check-prefix=UNKNOWN
+
----------------
What's with the `|&` ?


================
Comment at: test/tools/llvm-readobj/merged.test:13-16
+RUN: llvm-readobj -sr %p/Inputs/trivial.obj.elf-i386 | FileCheck %s
+RUN: llvm-readobj -sd %p/Inputs/trivial.obj.elf-i386 | FileCheck %s
+RUN: llvm-readobj -st %p/Inputs/trivial.obj.elf-i386 | FileCheck %s
+RUN: llvm-readobj -dt %p/Inputs/trivial.obj.elf-i386 | FileCheck %s
----------------
These should probably be in the corresponding tests for the appropriate dumps. I don't think we need them here.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:633
+  // The following two-letter aliases are only provided for readobj, as readelf
+  // allows these args to be grouped together.
+  static cl::alias SectionRelocationsShort(
----------------
I think this comment needs tweaking. As things stand, the args is ambiguous (i.e. is it referring to "-sd" or "-s" and "-d". I think you can replace "these args" with "single-letter args".


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:654
+
+  // Allow all single letter flags to be grouped together.
+  opts::AllShort.setFormattingFlag(cl::Grouping);
----------------
Could we come up with some way of enforcing this list to be extended by any new single letter aliases that are added? I don't know if it's possible, but one option might be to set grouping on by default, and disable it for llvm-readobj.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56629