aronsky added a comment. In D90221#2379793 <https://reviews.llvm.org/D90221#2379793>, @aaron.ballman wrote:
> 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). Thanks! I will take a look at that code. > 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. Thank you! I appreciate your feedback and your work on the issue. I'll take a better look at your patch (from a quick look, it definitely looks more organized and correct than mine), and see if I can understand - and fix - the duplicate traversal issue. 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
