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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:345
 
-static bool shouldKeep(object::SectionRef S) {
+struct FilterResult {
+  bool Keep;
----------------
grimar wrote:
> 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) {
> ...
> }
> };
> ```
> 
> 
> 
Wrapped just the struct per http://llvm.org/docs/CodingStandards.html#anonymous-namespaces


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:352
   if (FilterSections.empty())
-    return true;
+    return {/*Keep=*/true, /*Increment=*/true};
 
----------------
grimar wrote:
> I think we use a full variable name usually, i.e. Increment -> IncrementIndex
Sorry, I originally had the variable named `Increment` but forgot to update these comments.


================
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());
----------------
grimar wrote:
> Looking at this,
> should `ToolSectionFilter` just return `[(SectionRef&)Ref, (uint64_t )Index]` struct/pair instead? It seems could make the whole logic simper.
That's one of the paths I considered while writing this patch. A couple thoughts:
1) This is the only place that needs this counter value, and loop iteration in every other case (7 other places in this file, 1 in the MachO dumper) would now be a little more complicated even when callers don't care about the index, e.g. it would now be:
```
for (const SomeWrapperType &Foo : ToolSectionFilter(*Obj)) {
  const SectionRef &Section = Foo.Section;
```
2) llvm has `make_filter_range` which probably did not exist at the time this code was first written, and it would be nice to completely remove `SectionFilter` and `SectionFilterIterator` from llvm-objdump.h in favor of those standard llvm libraries, but AIUI in order to use that, the return type would need to be `SectionRef`, not some wrapper type. (I'm trying to do that in a separate branch, but I'm dealing with template woes).
3) But on the plus side, it does avoid out parameters which would be very nice.

I think 1&2 outweigh 3, so I'm leaning towards this approach. But I'm still exploring alternatives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68848