Sirraide wrote: > IF all 4 nodes are required to do ANY work here, I suspect we probably should > be combining things. Honestly, this patch is so much work, I don't see myself > being able to review this meaningfully in any reasonable amount of time.
Oh yeah, that’s perfectly understandable; I wouldn’t expect a single person to be able to review of it—at least definitely not w/o taking several breaks (which is also why I added a lot of reviewers ;þ). > I don't have a great suggestion on how to split it up, but I cannot see > myself getting through a review on this sufficient to have this > accepted/merged as-is. After thinking about this a bit more, I guess we could make introducing all of the AST nodes a separate patch? But 1. it feels weird to just add a bunch of ‘unused’ AST nodes, and 2. it might get annoying if we then find out that we actually want to change some of them as part of the remaining review process. Another thing that we could do is: *most* of the changes in `SemaStmt.cpp` are just an NFC refactor of the range-based `for` loop code that extracts a few parts so the iterating expansion statement code can reuse them; I *might* be able to make that a separate patch, though I’m not sure if that would actually be all that helpful. https://github.com/llvm/llvm-project/pull/165195 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
