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

[George Rimar via Phabricator via llvm-commits]

grimar marked 2 inline comments as done.
grimar added inline comments.


================
Comment at: include/llvm/ObjectYAML/yaml2obj.h:49
 
-Error convertYAML(Input &YIn, raw_ostream &Out, unsigned DocNum = 1);
+bool yaml2coff(COFFYAML::Object &Doc, raw_ostream &Out, ErrorHandler EH);
+bool yaml2elf(ELFYAML::Object &Doc, raw_ostream &Out, ErrorHandler EH);
----------------
sbc100 wrote:
> It seems like there is a separate change here which is to convert a lot of return codes from "int -> 0 for success" to "bool -> 1 for success".  Is that objectively better?    Is there an llvm policy for which is preferred?   Could this change be more precise if that part was split out? 
I believe it is better because we only use `1` and `0`. There are no another values => the valid type to represent it should be `bool`. I never heard about policies about the returning types in the libraries. And it is a bit hard to imagine one that would say to use `int` when we can use `bool` :)

I am OK to split it out I think, I am not sure if it can make this diff much shorter/easier to read, but I'll try.


================
Comment at: lib/ObjectYAML/yaml2obj.cpp:21
 
-Error convertYAML(yaml::Input &YIn, raw_ostream &Out, unsigned DocNum) {
-  // TODO: make yaml2* functions return Error instead of int.
-  auto IntToErr = [](int Ret) -> Error {
-    if (Ret)
-      return createStringError(errc::invalid_argument, "yaml2obj failed");
-    return Error::success();
-  };
+static void defaultErrorHandler(const Twine &Msg) {
+  WithColor::error() << Msg << "\n";
----------------
sbc100 wrote:
> I don't see this function referenced anywhere. 
Oh, you're right, thanks. I forgot to remove it. At first I experimenmted with the logic that used a default handler if no handler was provided, but then removed it. It seems that it is OK to always provide the custom one.


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

https://reviews.llvm.org/D67445