[PATCH] D90221: Include more attribute details when dumping AST in JSON

[Lev Aronsky via Phabricator via cfe-commits]

aronsky added a comment.

> 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:
> [...]
> 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.

Good point. I'm actually not an expert on the inner workings of clang structures (this is my first foray into LLVM), so I am not sure what causes this discrepancy. For some context - I'm working on a static analysis engine, and using clang to parse C/C++. The changes I'm proposing here are due to the fact I was missing attribute details in JSON, that are present in standard dumping (which, of course, isn't very consumable, compared to JSON).

> 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.

I concur that from a design standpoint, my solution is pretty lacking - it's more of a patch (no pun intended), rather than a fundamental fix to the issue at hand. That's also the reason for the extra whitespace you mentioned... I was trying to get the missing arguments with minimal changes, so I used existing code where available (in this case, code that dumped those arguments in TextNodeDumper). My hope was that the patch would be approved (since it does improve the output in a way, and doesn't break it), and someone more knowledgable in LLVM/clang internals would improve it further :). I would gladly take the responsibility, but it's not up to me - as far as my company is concerned, the changes are sufficient for the product, and further time spent on improvements is unwarranted. Maybe I can work on it in my free time, but I can't commit to it.


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

https://reviews.llvm.org/D90221