[PATCH] D53051: [llvm-tapi] initial commit, supports ELF text stubs
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 28 11:17:33 PST 2018
ruiu added inline comments.
================
Comment at: llvm/include/llvm/TextAPI/ELF/ELFStub.h:63
+ /// Sorts all symbols by name in ascending order.
+ void sortSymbolsByName();
+};
----------------
Instead of providing this function, you could make `Symbols`s type to `std::set` which is a sorted set.
================
Comment at: llvm/include/llvm/TextAPI/ELF/TBEHandler.h:34
+
+const VersionTuple TBEVersionCurrent = VersionTuple(1, 0);
+
----------------
const VersionTuple TBEVersionCurrent(1,0);
is more conventional.
================
Comment at: llvm/lib/TextAPI/ELF/ELFStub.cpp:13
+using namespace llvm;
+using namespace tapi::elf;
+
----------------
Let's use fully qualified name so that it doesn't look like we have `::tapi` namespace.
using namespace llvm::tapi::elf;
================
Comment at: llvm/lib/TextAPI/ELF/ELFStub.cpp:32
+void ELFStub::sortSymbolsByName() {
+ llvm::sort(Symbols, [](const ELFSymbol &LHS, const ELFSymbol &RHS) -> bool {
+ return LHS.Name < RHS.Name;
----------------
nit: I believe you can omit `-> bool`.
================
Comment at: llvm/unittests/TextAPI/ELFYAMLTest.cpp:24
+
+ StringRef Buf = StringRef(Data);
+
----------------
StringRef Buf(Data);
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53051/new/
https://reviews.llvm.org/D53051
More information about the llvm-commits
mailing list