[PATCH] D96883: Add support for JSON output style to llvm-symbolizer

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:58
+
+  void printErrorJSON(const std::string &Msg, const std::error_code &EC);
 };
----------------
jhenderson wrote:
> This doesn't feel like the right design choice here. I think a better thing to do would be to have a function like `DIPrinter &operator<<(Error E);` which can be used for reporting errors for all output formats.
I'd like to explain it. It was initially done in that way few diffs ago. I've suggested to add the `printErrorJSON` because it didn't feel right to me to have `operator<<(Error E)`, because having an error it is not a normal regular output case.
It feels to me that having a named function, rather than overloading a regular `operator<<` looks cleaner in this case: when used it emphasizes that the error handling is performed.

It is a bit subtle: having an error for JSON output is kind of normal, because we have a special output.
LLVM/GNU cases are different, we print the following currently:

`LLVMSymbolizer: error reading file: <reason>`

I.e. for the latter case we have a caller that reports an error (caller even could just call `exit(1)` there, if it wanted to, and I would assume it could be normal). For the JSON case we have a special printer logic,
and the caller should no nothing.

I've mentioned earlier that the error code doesn't seem to be very useful to have? If so we could have
something like `virtual bool onError(const std::string &Msg); { return false; }` for all output formats.
Then the logic could be like:

```
if (onError(toString(E)) {
 // Handled by JSON.
  return;
}

error(...); // GNU,LLVM
```

(Note: I believe that reporting `LLVMSymbolizer: error reading file: <reason>` from LLVM/GNU subclasses of DIPrinter is **not **a good thing. Because for them an error output is not a part of their output style, it is just an error. That is why I was assuming that the caller should report errors for them, like it does now).

And, of course, such logic from above only can only work if we do the refactoring you've suggested first ("to have a separate class per output format (JSON, GNU, LLVM), which share a common interface").
It wasn't clear to me that we might want to add more output styles in the future (I don't know a reason for that currently). So I've suggested the simplest less intrusive approach: having a special `printErrorJSON` method.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883