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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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;
----------------
I see `is` below so I'm thinking whether we should use `gets` here.


================
Comment at: lib/ObjectYAML/MachOEmitter.cpp:390
+  typedef void (MachOWriter::*writeHandler)(raw_ostream &);
   typedef std::pair<uint64_t, writeHandler> writeOperation;
   std::vector<writeOperation> WriteQueue;
----------------
Probably make a separate change to convert `typedef` to `using`.


================
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'
 
----------------
`yaml2obj: ` is prepended to a few error messages. In llvm-readobj/llvm-objcopy/llvm-objdump, I think we leave out the tool name?


================
Comment at: tools/yaml2obj/yaml2obj.cpp:56
+  if (EC) {
+    ErrHandler("can't open '" + OutputFilename + "': " + EC.message());
+    return 1;
----------------
or `failed to open`.

`failed to parse` is used in yaml2coff.


================
Comment at: unittests/ObjectYAML/YAML2ObjTest.cpp:77
+  bool Res = convertYAML(YIn, OS, ErrHandler);
+  ASSERT_FALSE(Res);
+  ASSERT_TRUE(Errors.size() == 2);
----------------
`Res` can be deleted.


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

https://reviews.llvm.org/D67445