aaron.ballman added a comment. In D90221#2374344 <https://reviews.llvm.org/D90221#2374344>, @aronsky wrote:
>> 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. I think the issue is that ASTNodeTraverser is visiting all of the attribute arguments (around ASTNodeTraverse.h:726) and your patch also visits them (as part of what's emitted from ClangAttrEmitter.cpp). > 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). Yup, this is a great use for `-ast-dump=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. Unfortunately, I don't think we can accept the patch as-is -- it's incomplete (misses some kinds of attribute arguments), has correctness issues (duplicates some kinds of attribute arguments), and would make it harder to maintain the code moving forward (with the layering issue). I can definitely understand not having a lot of time to investigate the right way to do this, so I spent a bit of time this afternoon working on a patch that gets partway to what I think needs to happen. I put up a pastebin for it here: https://pastebin.com/6ybrPVu6. It does not solve the issue of duplicate traversal, however, and I'm not certain I have more time to put into the patch right now. Perhaps one of us will have the time to take this the last mile -- I think the functionality is needed for JSON dumping. 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
