[PATCH] D143097: [NFC] add new function is64Bit for SymbolicFile class

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Some nits, but this basically looks fine so far. Do we have testing that exercises each of the new methods?



================
Comment at: llvm/include/llvm/Object/MachO.h:740
   StringRef getStringTableData() const;
-  bool is64Bit() const;
+
   void ReadULEB128s(uint64_t Index, SmallVectorImpl<uint64_t> &Out) const;
----------------
I'm not sure there's a need to move the declaration?


================
Comment at: llvm/include/llvm/Object/SymbolicFile.h:161
 
+  virtual bool is64Bit() const { return false; }
+
----------------
I think it would be better to make this a pure virtual function and force every implementation to specify the bitness of the object file (even if it's always 32-bit). Otherwise, when a new format comes along, it would be easy to forget to implement this function.


================
Comment at: llvm/include/llvm/Object/TapiFile.h:51-53
+  bool is64Bit() const override { return MachO::is64Bit(Arch); }
 
+  static bool classof(const Binary *v) { return v->isTapiFile(); }
----------------
Please leave these methods in the order they were in before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143097