[PATCH] D61672: [llvm-objcopy] Allow strip symtab in executables and DSOs

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

A number of my test comments haven't been addressed yet. Please do so.



================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:554-557
+static bool isPlaceholder(const Symbol *Sym) {
+  return Sym->Type == STT_NOTYPE && Sym->Binding == STB_LOCAL &&
+         Sym->Visibility == STV_DEFAULT && Sym->getShndx() == SHN_UNDEF;
+}
----------------
I'm not sure I see the point of this function. The symbol at index 0 is guaranteed to be the null symbol.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:559
+
+bool SymbolTableSection::isTrivial() const {
+  assert(!Symbols.empty());
----------------
I'd be inclined to call this "isEmpty" or simply "empty". Whilst not strictly empty, the null symbol is not significant.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1591
+Error Object::removeUnneededSections() {
+  // We can remove trivial symbol table from non-relocatable objects
+  if (isRelocatable() || SymbolTable == nullptr || !SymbolTable->isTrivial())
----------------
Missing trailing full stop.
"remove trivial" -> "remove an empty".


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1595
+
+  // .strtab can be used for section names, in such case we shouldn't
+  // remove it.
----------------
names, in such case -> names. In such a


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

https://reviews.llvm.org/D61672