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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

To separate concerns, it would be a good idea to put the DIPrinter refactoring in a separate prerequisite patch, that the JSON implementation patch depends on.

I'll probably have loads more comments once that is done, but it'll be easier to sift through them once it is.



================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:30
 
+template <typename T> struct DICommon {
+  std::string ModuleName;
----------------
This isn't a good type name. What is a "Common"? Would it make more sense to pass the common data as a separate argument to the non-common `Result`?


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:58
+  // Print only the error.
+  virtual void print(const DICommon<std::string> &Common) = 0;
+};
----------------
Perhaps `printError` is good, because the type signature in its current form doesn't make it clear that this is for error reporting.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:61
+
+class DIPrinterGeneric : public DIPrinter {
+private:
----------------
We should probably have `DIPrinterLLVM` and `DIPrinterGNU`. The two could largely share functionality under the hood, but I think it is a cleaner interface than one implementation of DIPrinter controlling all its output style, and the other having to look at another variable to choose between styles.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:97-98
+private:
+  void print(const std::string &ObjFile, uint64_t Address, int EC,
+             const std::string &ErrorMsg);
+
----------------
Use `StringRef`, not `const std::string &`.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:111
 };
+
 }
----------------
Make sure to run clang-format on all your new code.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:172
+
+//---------------------------------------------------------------------------------------------------------
+
----------------
Delete this. We don't use arbitrary comment markers to divide up files in LLVM coding style.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:176
+
+static void toJSON(json::OStream &J, const DILineInfo &Info) {
+  J.objectBegin();
----------------
You seem to have ignored my earlier comments re. printing everything?


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:188
+    J.attribute("FileName", Info.FileName);
+  // Print only valid line and column to reduce noise in the output
+  if (Info.Line)
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883