MyDeveloperDay added a comment. When I spoke with one of the code owners, they seemed to have a problem accepting this review based on there not being a general description/understanding of how this algorithm works.
Having said that, all of the "AlignToken" based functions (alignConsecutiveXXXX) are hard to understand unless you take the AlignTokens() function as read, and just look at the Lambda. As a relative new comer, I find some areas of clang-format hard to understand, I tend to learn them as and when I need to, but the gist of AlignTokens is in its comment, but with comments like 'There is a non-obvious subtlety' probably means you really need to be in it and inside a debugger to see what is going on. For this review I actually don't personally see what is Macro specific about AlignMacroSequence(), it seems to be almost identical to AlignTokenSequence without the scope and comment handling, (which actually makes it a lot simpler), my feeling is it could be used for more than just this, maybe pulling this out into its own set of functions wasn't the correct thing to do, and actually the original design might have been better, but this solution seems more simplistic and because its isolated means it doesn't break the other functions. All I feel I can add to the review process, is that perhaps a few more comments explaining the subtlety of the various "if statements" in both the Lambda and in the main alignment might help bring some clarify.. but to be honest I'm OK with how it works and I'd like it in. Ultimately I'd also like to see this revision land, especially as its off by default, I would like to see some ability to be able to set a Minimum Column distance to the value (this would allow Visual Studio to not keep playing the hokey-cokey (https://en.wikipedia.org/wiki/Hokey_cokey) with resource.h files.), but perhaps that is for a later change once this is accepted. From my perspective it LGTM, if the code owners are present they should really rubber stamp it, If not well... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D28462/new/ https://reviews.llvm.org/D28462 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
