[PATCH] D79939: Implement --depedent-libraries for GNU output

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test:7
 
 # RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj --dependent-libraries %t1 | \
----------------
Probably it worth to mention that `--dependent-libraries` is not implemented in GNU at the moment of 14 may 2020.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test:9
+# RUN: llvm-readobj --dependent-libraries %t1 | \
+# RUN:   FileCheck %s --check-prefix=LLVM --strict-whitespace --match-full-lines
+# RUN: llvm-readelf --dependent-libraries %t1 | \
----------------
Add `--implicit-check-not="DependentLibs"` for consistency with below line?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test:41
+# RUN: llvm-readelf --dependent-libraries %t2 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=MIX-GNU -DFILE=%t2
+
----------------
These could be 2 lines, seems no need to wrap.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:729
   virtual void printDependentLibs(const ELFFile<ELFT> *Obj) = 0;
+  void printDependentLibsHelper(
+      const ELFFile<ELFT> *Obj,
----------------
Can this be a protected method?

I'd also not mix virtual vs non-virtual methods.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5328
+
+    for (const uint8_t *S = Contents.begin(), *I = Contents.begin(),
+                       *E = Contents.end();
----------------
`S` is not used as `for` variable/condition. Seems that inlining `Contents.begin()` would look cleaner.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5347
+  StringRef SecName;
+  uint64_t SecOffset;
+  auto PrintSection = [&]() {
----------------
Seems you can replace `SecName` and `SecOffset` with a new `NameOffset` variable.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5364
+          PrintSection();
+        }
+        SectionStarted = true;
----------------
You do not need to wrap a single line into brackets.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5378
+      },
+      [&SecEntries](StringRef Lib, uint64_t Offset) {
+        SecEntries.push_back(NameOffset{Lib, Offset});
----------------
It would be easier to use `&` here too, as this lambda is not supposed to be used outside of the scope of this method.
In this case using just `&` reads IMO better in compare with explicit listing of variables.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5380
+        SecEntries.push_back(NameOffset{Lib, Offset});
+      });
+  if (SectionStarted)
----------------
I'd probably move out these helper functions. It is a bit hard to read them inlined.
Naming them should help for readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79939