[PATCH] D67445: [yaml2obj/ObjectYAML] - Cleanup the error reporting API, add custom errors handlers.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: include/llvm/ObjectYAML/yaml2obj.h:47
 
-int yaml2coff(COFFYAML::Object &Doc, raw_ostream &Out);
-int yaml2elf(ELFYAML::Object &Doc, raw_ostream &Out);
-int yaml2macho(YamlObjectFile &Doc, raw_ostream &Out);
-int yaml2minidump(MinidumpYAML::Object &Doc, raw_ostream &Out);
-int yaml2wasm(WasmYAML::Object &Doc, raw_ostream &Out);
+typedef llvm::function_ref<void(const Twine &Msg)> ErrorHandler;
 
----------------
jhenderson wrote:
> using instead of typedef maybe?
Ok.


================
Comment at: lib/ObjectYAML/COFFEmitter.cpp:84
         if (str.size() > 7) {
-          errs() << "String table got too large\n";
+          ErrHandler("string table got too large");
           return false;
----------------
MaskRay wrote:
> I see `is` below so I'm thinking whether we should use `gets` here.
I guess both of spellings make sense. But these messages are far from ideal. I think reporting "section alignment is too large" or "string table is/got too large" without mentioning at least the name of a section is not too usefull anyways. I'd leave them alone for now.


================
Comment at: lib/ObjectYAML/MachOEmitter.cpp:18
 #include "llvm/ObjectYAML/yaml2obj.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/LEB128.h"
----------------
jhenderson wrote:
> I haven't yet looked at the other files, but this seems like an unnecessary include, right?
Yes, removed.


================
Comment at: lib/ObjectYAML/MachOEmitter.cpp:390
+  typedef void (MachOWriter::*writeHandler)(raw_ostream &);
   typedef std::pair<uint64_t, writeHandler> writeOperation;
   std::vector<writeOperation> WriteQueue;
----------------
MaskRay wrote:
> Probably make a separate change to convert `typedef` to `using`.
LLVM heavily uses typedefs too, so I guess it is not a real problem.
New code probably should use `using` though.


================
Comment at: lib/ObjectYAML/WasmEmitter.cpp:130
+
+void WasmWriter::writeInitExpr(raw_ostream &OS, const wasm::WasmInitExpr &InitExpr) {
   writeUint8(OS, InitExpr.Opcode);
----------------
jhenderson wrote:
> clang-format?
Yep.


================
Comment at: test/tools/yaml2obj/dynsymtab-implicit-sections-size-content.yaml:34
 
-# CASE2: error: cannot specify both `Size` and `DynamicSymbols` for symbol table section '.dynsym'
-# CASE2: error: yaml2obj failed
+# CASE2: yaml2obj: error: cannot specify both `Size` and `DynamicSymbols` for symbol table section '.dynsym'
 
----------------
MaskRay wrote:
> `yaml2obj: ` is prepended to a few error messages. In llvm-readobj/llvm-objcopy/llvm-objdump, I think we leave out the tool name?
I added it here because test on the left had "yaml2obj failed" message and now we do not have it.
So I wanted to show the whole difference.
Generally I think we should not include testing of the tool name everywhere.

I added it to tests like "invalid-docnum.test" and "empty-or-invalid-doc.yaml" below though, since they
test the basic functionality and reporting the tool name is a basic thing that should be checked at least for basic messages perhaps.

But generally I do not have a strong feeling about how we should test this, so can remove it, though do not see a problem to keep it too.


================
Comment at: tools/yaml2obj/yaml2obj.cpp:56
+  if (EC) {
+    ErrHandler("can't open '" + OutputFilename + "': " + EC.message());
+    return 1;
----------------
MaskRay wrote:
> or `failed to open`.
> 
> `failed to parse` is used in yaml2coff.
Ok.


================
Comment at: unittests/ObjectYAML/YAML2ObjTest.cpp:77
+  bool Res = convertYAML(YIn, OS, ErrHandler);
+  ASSERT_FALSE(Res);
+  ASSERT_TRUE(Errors.size() == 2);
----------------
MaskRay wrote:
> `Res` can be deleted.
Probably not, because we want to test that `convertYAML` doesn't return `true` when there are errors.


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

https://reviews.llvm.org/D67445