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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Looks mostly good to me. I'd suggest to focus on refining the comments in test cases.
Currenty it is often not clear what is tested and what is expected result.
E.g:

  ## Test JSON output style of DIInliningInfo.
  # RUN: llvm-symbolizer --output-style=JSON -e %p/Inputs/addr.exe < %p/Inputs/addr.inp \
  # RUN:   | FileCheck %s --check-prefix=INLINE --strict-whitespace --match-full-lines --implicit-check-not={{.}}
  
  #      INLINE:{"Error":{"Code":22,"Message":"unable to parse arguments: some text"}}
  # INLINE-NEXT:{"Frames":[
  # INLINE-NEXT:{"FunctionName":"inctwo","StartFileName":"/tmp{{/|\\\\}}x.c","StartLine":2,"FileName":"/tmp{{/|\\\\}}x.c","Line":3,"Column":3}
  # INLINE-NEXT:,{"FunctionName":"inc","StartFileName":"/tmp{{/|\\\\}}x.c","StartLine":6,"FileName":"/tmp{{/|\\\\}}x.c","Line":7}
  # INLINE-NEXT:,{"FunctionName":"main","StartFileName":"/tmp{{/|\\\\}}x.c","StartLine":12,"FileName":"/tmp{{/|\\\\}}x.c","Line":14}]}
  # INLINE-NEXT:{"Error":{"Code":22,"Message":"unable to parse arguments: some text2"}}

The JSON contains 2 errors and the declaration of `Frames`, it raises a question, like "why there are errors in the output"?
If you want to test the case where there are errors around a some normal output, then you should explain what exactly you are doing/testing.

Also, see my comment for `output-style-json-data.test`. Seems using of class names, like `DIInliningInfo` in comments
is not very helpful: it doesn't explain anything for a reader who doesn't know the code well.



================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:34
 
+std::string hex(uint64_t V) { return ("0x" + Twine::utohexstr(V)).str(); }
+
----------------
I am not sure I'd introduce this helper: it is common to just inline the code like `"0x" + Twine::utohexstr(V)`.
Though if you want it, probably the better name is `toHex` then.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-data.test:11
+
+## Test JSON output style of DIGlobal in case of error.
+# RUN: llvm-symbolizer "DATA %t-no-file.o 0" --output-style=JSON \
----------------
Note that you already have exactly the same comment at line 5.
Currently it is not very clear from comments what tests are doing.

I think it is better to not to refer to `DIGlobal` and other class names in comments.
They are a part of internal implementation and doesn't explain well what this test does for a reader who
are not familar with the code.

You can just explain what is exactly validated. E.g. this one comment could be like:
"Test that we print a valid error message to JSON when the input file is not present"

This applies to all test cases as far I can see.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-frame.test:41
+##
+## clang++ --target=i386-linux-gnu frame.cpp -g -std=c++11 -S -o frame.s 
+ 
----------------
Adding an exact clang version used might be helpfull.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json.test:1
+## This test checks JSON output for CODE.
+
----------------
Probably you should name this test case as `output-style-json-code.test`?
It seems a bit more consistent with `output-style-json-frame.test` and `output-style-json-data.test`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883