[PATCH] D96289: Add support for YAML output style to llvm-symbolizer

[Alex Orlov via Phabricator via llvm-commits]

aorlov marked 8 inline comments as done.
aorlov added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:43
+    IO.mapOptional("Source", Value.Source);
+    IO.mapOptional("FunctionName", Value.FunctionName, llvm::DILineInfo::BadString);
+    IO.mapOptional("StartFileName", Value.StartFileName, llvm::DILineInfo::BadString);
----------------
jhenderson wrote:
> Please reformat using clang-format, as per the linter comment.
> 
> Why add the `llvm::` namespace qualifier? You're already in the llvm namespace and use `DILineInfo` in the function signature without it.
> 
> Same goes elsewhere below.
I added redundant llvm:: to see if that would make a pre-commit Debian bot happy. It didn’t, so I removed.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:116
+    llvm::yaml::Output YOut(OS);
+    DILineInfo Val = Info; // copy: YOut<< requires mutability.
+    YOut << Val;
----------------
jhenderson wrote:
> Re. the comment - I wonder if that's a bug in the `<<` implementation? Does it need a `const` adding to its signature, or does that break other things?
You are right, this lookd strange, however changing this is out of the scope of this patch.
My solution is based on the following code
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/index/YAMLSerialization.cpp#L493



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96289