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

[Alex Orlov via Phabricator via llvm-commits]

aorlov added inline comments.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-frame.ll:1
+; This test checks JSON output for FRAME.
+
----------------
jhenderson wrote:
> Does this need a `REQUIRES: aarch64-registered-target` or equivalent?
It does not require any ARM target. 
Note this test is passed on x64 Windows and x64 Debian.



================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-frame.ll:20
+
+; Resolve valid address.
+; RUN: llvm-symbolizer "FRAME %t.o 0" --output-style=JSON | \
----------------
jhenderson wrote:
> Perhaps highlight that you are testing both 0 and non-zero frame offsets with a comment.
I have updated output-style-json-frame.ll.
Now it contains 0, non-zero and empty (missing) TagOffset to cover all possible cases.
I added a comment too.



================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-frame.ll:25
+
+target triple="aarch64--"
+
----------------
jhenderson wrote:
> This code could probably do with some comments highlighting what the key elements are, so that future changes don't lose them.
There are no any key elements. I just declared 3 variables with different type, size and all possible TagOffset per your requests.
I personally think it is overkill, and does not really belong to the JSON patch, as there is nothing JSON specific there. 
It seems a test for the symbolize library and the symbolizer itself.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883