*From: *MyDeveloperDay via Phabricator <revi...@reviews.llvm.org> *Date: *Fri, May 10, 2019 at 12:30 PM *To: * <velocit...@gmail.com>, <djas...@google.com>, <kli...@google.com>, < krasi...@google.com>, <sammcc...@google.com>, <eknyqu...@gmail.com> *Cc: * <jkor...@apple.com>, <gardne...@llnl.gov>, <fgr...@noexcept.net>, < marcosbe...@gmail.com>, <lambdas...@gmail.com>, <dougp...@gmail.com>, < shua2...@gmail.com>, <jacob.kel...@gmail.com>, <mi...@mikelward.com>, < vit9...@avp.su>, <razielm...@gmail.com>, <jgreg...@atiba.com>, < mydeveloper...@gmail.com>, <bob_basket...@hotmail.com>, < bderickson+llvm...@gmail.com>, <anantha.keshava...@gmail.com>, < vee...@veegee.org>, <alexan...@phi.nz>, <micas...@cisco.com>, < up752...@myport.ac.uk>, <erik.john...@live.ca>, <lassi.niemi...@gmail.com>, <tiena2...@gmail.com>, <marcos.ho...@udc.es>, <timothee.fi...@gmail.com>, < jorisae...@gmail.com>, <katya.roman...@sony.com>, <sc...@chickadee.tech>, < llvm-comm...@lists.llvm.org>, <wdie...@illinois.edu>, <mze...@vmware.com>, < zarevucky.j...@gmail.com>, <mdanie...@apple.com>, <rian.sander...@gmail.com>, <mcud...@gmail.com>, <cfe-commits@lists.llvm.org>, <caner_kos...@hotmail.com >
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.. That is basically what I thought. > 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... > If you believe you understand this code well enough to LG it, I'm fine with it going in. I think we need one reviewer spending enough to time understand why the code is the way it is, and given the quality of code I've seen from you, I do trust your ability to understand code :) > > > 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
