[PATCH] D27355: [WebAssembly] Add wasm support for llvm-readobj

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: tools/llvm-readobj/WasmDumper.cpp:85-88
+  const WasmObjectFile *WasmObj = dyn_cast<WasmObjectFile>(Obj);
+  if (!WasmObj)
+    return readobj_error::unsupported_obj_file_format;
+
----------------
sbc100 wrote:
> davide wrote:
> > Can this always happen? You enter here under `Obj->isWasm()`.
> > If there's a legitimate where this can happen, I would add a test (if you don't mind).
> > Also, not entirely sure if `unsupported_obj_file_format` is the best diagnostic we can emit. Maybe `broken_file`? (I suspect this is consistent with what other dumpers are doing, if so, leave it as is but please add a test to exercise this code).
> This is exactly what the MachO and COFF dumpers do yes, and basically the same as the ELF dumper.
> 
> You are correct that in practice there is no way this can happen, at least not by running the tool itself.   In light of this I don't see how I can write a test to tickle this, and I think perhaps an assert would be more appropriate here?  
Yes, that's what I would do.
Probably better not cargo culting dubious code from other drivers.


https://reviews.llvm.org/D27355