[PATCH] D26172: [WebAssembly] Add llvm-objdump support for wasm file format

[Davide Italiano via llvm-commits]

davide added inline comments.


================
Comment at: include/llvm/Object/Wasm.h:31
+  const wasm::WasmSection *getWasmSection(const SectionRef &Section) const;
+  static inline bool classof(const Binary *v) { return v->isWasm(); }
+
----------------
sbc100 wrote:
> davide wrote:
> > I think `inline` inside class definition is not necessary (check elsewhere).
> All the other headers in include/llvm/Object do it this way
This doesn't necessarily mean they're right.


================
Comment at: lib/Object/WasmObjectFile.cpp:71
+  if (Header.Magic != StringRef("\0asm", 4)) {
+    Err = make_error<GenericBinaryError>("Bad magic number",
+                                         object_error::parse_failed);
----------------
sbc100 wrote:
> davide wrote:
> > Can't this just be a `StringError` instead of a `GenericBinaryError`? 
> GenericBinaryError seems to make sense since this is an problem with binary file.  Its a subclass of BinaryError which I think is that kind of error we want here.
> 
> From the docs from GenericBinaryError:
> 
> /// For errors that don't require their own specific sub-error (most errors)
> /// this class can be used to describe the error via a string message.
> 
> MachOObjectFile.cpp uses it too.
ehm, feels quite a bit overabstraction to me.
In this case, you don't need anything more than a StringError (as we do for ELF).


https://reviews.llvm.org/D26172