[PATCH] D11487: llvm-objdump --section, -j section filtering

[Filipe Cabecinhas]

filcab added a subscriber: filcab.
filcab added a comment.

Just some nitpicky details.
I also prefer to have empty lines before class definitions, but that might just be due to the way I use my editor.

Thank you,
Filipe


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:204
@@ +203,3 @@
+  void ScanPredicate() {
+    while (Iterator != End && Predicate(*Iterator)) {
+      ++Iterator;
----------------
We're *not* keeping the ones where the predicate returns true?
I'd prefer to call it `FilterOut`, or reverse this condition.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:237
@@ +236,3 @@
+                         if (error) {
+                           return error.value();
+                         }
----------------
Is this an appropriate value to return?

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:243
@@ +242,3 @@
+                         }
+                         return 1;
+                       },
----------------
`return std::find(Sections.begin(), Sections.end(), String) == Sections.end();`

(or `!= Sections.end()` if you switch the predicate around)

================
Comment at: tools/llvm-objdump/llvm-objdump.h:28
@@ -27,3 +27,3 @@
 extern cl::list<std::string> MAttrs;
-extern cl::list<std::string> DumpSections;
-extern cl::opt<bool> Disassemble;
+extern cl::list<std::string> Sections;
+extern cl::opt<bool> Disassemble;
----------------
Please split this in two commits: Renaming (and moving between .cpp files) `DumpSections` to `Sections`, and the the functional change.

================
Comment at: tools/llvm-objdump/llvm-objdump.h:29
@@ -30,1 +28,3 @@
+extern cl::list<std::string> Sections;
+extern cl::opt<bool> Disassemble;
 extern cl::opt<bool> DisassembleAll;
----------------
Are you changing whitespace in line 29? Adding `\r`? (The diff on phabricator looks weird)
If you're changing WS, please split that to another commit. If you're adding `\r`, please don't. :-)


Repository:
  rL LLVM

http://reviews.llvm.org/D11487