[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

[Manuel Klimek via llvm-commits]

*From: *MyDeveloperDay via Phabricator <reviews at reviews.llvm.org>
*Date: *Fri, May 10, 2019 at 12:30 PM
*To: * <velocityra at gmail.com>, <djasper at google.com>, <klimek at google.com>, <
krasimir at google.com>, <sammccall at google.com>, <eknyquist at gmail.com>
*Cc: * <jkorous at apple.com>, <gardner48 at llnl.gov>, <fgross at noexcept.net>, <
marcosbento at gmail.com>, <lambdasnow at gmail.com>, <dougpuob at gmail.com>, <
shua2727 at gmail.com>, <jacob.keller at gmail.com>, <mikel at mikelward.com>, <
vit9696 at avp.su>, <razielmine at gmail.com>, <jgregory at atiba.com>, <
mydeveloperday at gmail.com>, <bob_basketbal at hotmail.com>, <
bderickson+llvmorg at gmail.com>, <anantha.keshava.bs at gmail.com>, <
veegee at veegee.org>, <alexandro at phi.nz>, <micasnyd at cisco.com>, <
up752724 at myport.ac.uk>, <erik.johnson at live.ca>, <lassi.niemisto at gmail.com>,
<tiena2cva at gmail.com>, <marcos.horro at udc.es>, <timothee.fivaz at gmail.com>, <
jorisaerts at gmail.com>, <katya.romanova at sony.com>, <scott at chickadee.tech>, <
llvm-commits at lists.llvm.org>, <wdietz2 at illinois.edu>, <mzeren at vmware.com>, <
zarevucky.jiri at gmail.com>, <mdaniels2 at apple.com>, <rian.sanderson at gmail.com>,
<mcuddie at gmail.com>, <cfe-commits at lists.llvm.org>, <caner_kose98 at 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190510/1f840d6e/attachment.html>