aaron.ballman added a comment.
In D90221#2358469 <https://reviews.llvm.org/D90221#2358469>, @aronsky wrote: > In D90221#2357060 <https://reviews.llvm.org/D90221#2357060>, @aaron.ballman > wrote: > >> In D90221#2356110 <https://reviews.llvm.org/D90221#2356110>, @aronsky wrote: >> >>> In D90221#2356062 <https://reviews.llvm.org/D90221#2356062>, @lebedev.ri >>> wrote: >>> >>>> Are there tests missing? >>> >>> Quite possible. I followed the trail of the existing functions to figure >>> out the difference between JSON and textual dumping, and tried replicating >>> everything in a manner similar to the existing code. I haven't run into any >>> tests, but that's probably because I wasn't looking for those. I'll add the >>> appropriate tests ASAP. >> >> FWIW, the tests for dumping JSON live in `clang\test\AST` and typically have >> a `-json` extension on them. There is a helper script named >> `gen_ast_dump_json_test.py` that can be used to generate the expected output >> from the test. > > Thanks, I'll take a look at the Python script, that'll be helpful! > > Those functions do look out of place, but they are actually called via > polymorphism (I wish I could point to the exact location - it wasn't easy > figuring that out in the first place, and the actual work was done about a > month ago, I just got to publishing the PR yesterday). The code that calls > these functions is emitted at `writeDump` (in > `clang/utils/TableGen/ClangAttrEmitter.cpp`) - which, in turn, is called by > `EmitClangAttrJSONNodeDump` and `EmitClangAttrTextNodeDump` in the same file. It seems surprising to me that you'd have to add those declarations; I think that's a layering violation. There's something somewhat strange going on here that I think is related. Given: [[clang::annotate("testing")]] void func1(); [[gnu::visibility("hidden")]] void func2(); I get: { "id": "0x2527bc0", "kind": "FunctionDecl", "loc": { "offset": 36, "file": "F:\\Aaron Ballman\\Desktop\\test.cpp", "line": 1, "col": 37, "tokLen": 5 }, "range": { "begin": { "offset": 31, "col": 32, "tokLen": 4 }, "end": { "offset": 42, "col": 43, "tokLen": 1 } }, "name": "func1", "mangledName": "?func1@@YAXXZ", "type": { "qualType": "void ()" }, "inner": [ { "id": "0x2527c60", "kind": "AnnotateAttr", "range": { "begin": { "offset": 2, "col": 3, "tokLen": 5 }, "end": { "offset": 27, "col": 28, "tokLen": 1 } }, "args": [ " \"testing\"" ], "inner": [ { "id": "0x24bcfd8", "kind": "StringLiteral", "range": { "begin": { "offset": 18, "col": 19, "tokLen": 9 }, "end": { "offset": 18, "col": 19, "tokLen": 9 } }, "type": { "qualType": "const char [8]" }, "valueCategory": "lvalue", "value": "\"testing\"" } ] } ] }, { "id": "0x2527de8", "kind": "FunctionDecl", "loc": { "offset": 81, "line": 2, "col": 36, "tokLen": 5 }, "range": { "begin": { "offset": 76, "col": 31, "tokLen": 4 }, "end": { "offset": 87, "col": 42, "tokLen": 1 } }, "name": "func2", "mangledName": "?func2@@YAXXZ", "type": { "qualType": "void ()" }, "inner": [ { "id": "0x2527e88", "kind": "VisibilityAttr", "range": { "begin": { "offset": 48, "col": 3, "tokLen": 3 }, "end": { "offset": 72, "col": 27, "tokLen": 1 } }, "args": [ " Hidden" ] } ] } Notice how the annotate attribute has an `inner` array with the argument inside of it while the visibility attribute does not? Ideally, we'd either use `args` or `inner` but not a mixture of the two. I was imagining the design here to be that the JSON node dumper for attributes would open a new JSON attribute named "args" whose value is an array of arguments, then you'd loop over the arguments and `Visit` each one in turn as the arguments are children of the attribute AST node in some respects. For instance, consider dumping the arguments for the `enable_if` attribute, which accepts arbitrary expressions -- dumping a type or a bare decl ref is insufficient. ================ Comment at: clang/test/AST/ast-dump-stmt-json.cpp:1465 +// CHECK-NEXT: "args": [ +// CHECK-NEXT: " Default" +// CHECK-NEXT: ] ---------------- There's extra leading whitespace here that seems like it shouldn't be there. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90221/new/ https://reviews.llvm.org/D90221 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
