labath edited reviewers, added: zturner, davide; removed: labath.
labath added a comment.
I am not sure I'll have the resources to see this review through, so I'd prefer
to leave this to someone else.
The thoughts I have had so far are:
- the patch is very big and probably runs afoul of the "you shall develop
incrementally" section in the LLVM developer policy. At least the JSON parts
should be split off into a separate patch and tested independently.
- however, the choice of the JSON library is also an open question. We
currently have at least three options to choose from:
- llvm/Support/JSON.h
- lldb/Utility/JSON.h
- debugserver/source/JSON.h
Of these, the third one is the one I'd least expect to be used here.
- Since this is essentially starting a new-subproject, I think it's in place to
discuss various conventions. E.g., right now, this seems to use a mixture of
UpperCamel and snake_case, and so isn't very consistent with neither llvm nor
lldb naming conventions.
================
Comment at:
packages/Python/lldbsuite/test/tools/lldb-vscode/attach/TestVSCode_attach.py:47
+
+ @no_debug_info_test
+ def test_by_pid(self):
----------------
You might want to set `NO_DEBUG_INFO_TESTCASE = True` in the base
`VSCodeTestCaseBase` to avoid these. I guess none of these tests should be
debug-info dependent, right?
================
Comment at: tools/lldb-vscode/lldb-vscode.cpp:1688
+
+void __attribute__((format(printf, 2, 3)))
+send_formatted_output(OutputType o, const char *format, ...) {
----------------
gcc-specific attribute
https://reviews.llvm.org/D50365
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
