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

[Sam Clegg via llvm-commits]

sbc100 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(); }
+
----------------
davide wrote:
> 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.
Hmm, seems like the codebase is quite divided on this one:

$ git grep classof ./include | grep inline | wc -l
230
$ git grep classof ./include | grep -v inline | wc -l
257

Perhaps I should propose a cleanup  CL? 

In the mean time I'll drop inline here I guess?


================
Comment at: lib/Object/WasmObjectFile.cpp:71
+  if (Header.Magic != StringRef("\0asm", 4)) {
+    Err = make_error<GenericBinaryError>("Bad magic number",
+                                         object_error::parse_failed);
----------------
davide wrote:
> 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).
OK.. will change.


https://reviews.llvm.org/D26172