[PATCH] D79939: Implement --depedent-libraries for GNU output
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 14 06:58:05 PDT 2020
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
More information about the llvm-commits
mailing list