[PATCH] D45851: [llvm-objcopy] Fix sh_link for more types of sections

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.
Herald added a reviewer: javed.absar.


================
Comment at: tools/llvm-objcopy/Object.cpp:370
 
-bool SectionWithStrTab::classof(const SectionBase *S) {
-  return isa<DynamicSymbolTableSection>(S) || isa<DynamicSection>(S);
+bool Section::classof(const SectionBase *S) {
+  return S->Type == ELF::SHT_DYNSYM || S->Type == ELF::SHT_DYNAMIC ||
----------------
jakehehrlich wrote:
> no  additional fields are exposed by Section so there is no reason to use this cast. Additionally getting this method right seems hard/impossible to me. The point of this class is to be a catch-all for the cases we don't have specified behavior for so it really shouldn't ever make sense to cast it out to something more specific. Can we just remove method instead of trying to implement it correctly? I think I made a mistake in not removing SectionWithStrTab at some point in the code (it was needed at one point but I'm not sure it is needed any longer).
If it is needed, perhaps this is an indication that we should use a "Kind"-based system, similar to what LLD, for example, does - define an enum in the base class, and each different class type that can be cast to initialises a member to a value from that enum. I don't like the required change to this version of classof at all.


Repository:
  rL LLVM

https://reviews.llvm.org/D45851