[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 inline 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());
----------------
rupprecht wrote:
> 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.
I see. I have no much better/different ideas atm. Perhaps the current approach is OK for now.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:376
+  // increment so the indexing is stable.
+  return {/*Keep=*/is_contained(FilterSections, SecName),
+          /*IncrementIndex=*/true};
----------------
Can we have a test for this logic? I.e. for a case when `Keep=false`, `IncrementIndex=true`.
(Doesn't seem we have it)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68848