[PATCH] D59545: [llvm-readobj] Add new helper function `getSymbolVersionByIndex()`
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 28 03:10:01 PDT 2019
jhenderson added inline comments.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:650
+ unwrapOrError(ObjF->getELFFile()->template getEntry<Elf_Versym>(
+ dot_gnu_version_sec, EntryIndex));
+ return StringRef(
----------------
You don't need to do this now, but could you in a separate patch please replace dot_gnu_version_sec and similar with LLVM-style names?
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:672
+std::string ELFDumper<ELFT>::getSymbolVersionByIndex(
+ StringRef StrTab, const uint32_t SymbolVersionIndex, bool &IsDefault) const {
+ size_t VersionIndex = SymbolVersionIndex & VERSYM_VERSION;
----------------
I think it's more in keeping with existing style not to bother with the const on non-reference parameters.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:682
+
+ // Lookup this symbol in the version table
+ LoadVersionMap();
----------------
Nit: missing full stop.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:688
+
+ // Get the version name string
+ size_t NameOffset;
----------------
Ditto.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:700
+ reportError("Invalid string offset");
+ return std::string(StrTab.data() + NameOffset);
+}
----------------
Is std::string the right type to be returning here? This is a pointer into an existing block of strings, so I'd think a StringRef would be sufficient.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59545/new/
https://reviews.llvm.org/D59545
More information about the llvm-commits
mailing list