[PATCH] D49979: [llvm-objcopy] Add --dump-section

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:475
+  if (!Config.DumpSection.empty()) {
+    for (const auto &Flag : Config.DumpSection) {
+      auto SecPair = Flag.split("=");
----------------
jakehehrlich wrote:
> paulsemel wrote:
> > jhenderson wrote:
> > > jakehehrlich wrote:
> > > > You can just iterate over these instead of checking.
> > > To be fair, this is the same as how the other options have been implemented.
> > Yes, my point was to follow what's being done in the rest of the code, but I can change I don't really matter.
> Ah indeed. I think I've decided I find that silly. Do it this way for now. I might change that in the future. I remember the review where we did this.
yeah, i remember that discussion - at some point (and probably for a particular place - i don't remember that exactly) i thought this would provide some benefits 
(like early skipping some expensive checks), but later I took a closer look - in comparison with I/O + memory allocations + ELF parsing everything else in llvm-objcopy is almost invisible, so yeah, i think that wasn't right and code clarity is more important here. I think in any case we can refactor it on a separate diff (and it's really easy to do / not a big deal).


Repository:
  rL LLVM

https://reviews.llvm.org/D49979