[PATCH] D68848: [llvm-objdump] Use a counter for llvm-objdump -h instead of the section index.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

A few ideas/suggestions.



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:345
 
-static bool shouldKeep(object::SectionRef S) {
+struct FilterResult {
+  bool Keep;
----------------
It's common to wrap such things (helper types) into anonymous namespaces:
I think you can also add a helper function too:

```
namespace {
struct FilterResult {
  bool Keep;
  bool IncrementIndex;
};

FilterResult checkSectionFilter(object::SectionRef S) {
...
}
};
```





================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:347
+  bool Keep;
+  bool IncrementIndex;
+};
----------------
I'd comment these fields. It wasn't obvious to me what they are used for.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:352
   if (FilterSections.empty())
-    return true;
+    return {/*Keep=*/true, /*Increment=*/true};
 
----------------
I think we use a full variable name usually, i.e. Increment -> IncrementIndex


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:374
+  // zero (after the unsigned wrap).
+  if (Idx != nullptr)
+    *Idx = UINT64_MAX;
----------------
nit: `if (Idx)` would be shorer.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:378
+      [Idx](object::SectionRef S) {
+        auto Result = checkSectionFilter(S);
+        if (Idx != nullptr && Result.IncrementIndex)
----------------
Please avoid using auto for return types that are not obvious.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1699
+  uint64_t Idx;
+  for (const SectionRef &Section : ToolSectionFilter(*Obj, &Idx)) {
     StringRef Name = unwrapOrError(Section.getName(), Obj->getFileName());
----------------
Looking at this,
should `ToolSectionFilter` just return `[(SectionRef&)Ref, (uint64_t )Index]` struct/pair instead? It seems could make the whole logic simper.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:86
+// filtered (e.g. symbol tables).
+SectionFilter ToolSectionFilter(llvm::object::ObjectFile const &O,
+                                uint64_t *Idx = nullptr);
----------------
I think this needs a full comment (it does not give an information about what this helper avtually do atm):

I.e. would be nice to see something like:
```
// Function is used to ...
// Idx is a optional output parameter that ...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68848