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

Thu Feb 21 08:28:13 PST 2019

klimek added a comment.

In D28462#1405855 <https://reviews.llvm.org/D28462#1405855>, @MyDeveloperDay wrote:

> In D28462#1405711 <https://reviews.llvm.org/D28462#1405711>, @klimek wrote:
>
> > In D28462#1405360 <https://reviews.llvm.org/D28462#1405360>, @MyDeveloperDay wrote:
> >
> > > I'm not a code owner here but we seem to be lacking people who are either available, able, willing or allowed to approve code reviews for some of the code in the clang-tooling (not just clang format).  The code owners do an extremely good job at doing a deep review way beyond what many of the rest of us are able to do..  They've been doing this for years having written may of these tools in the first place, but I suspect other external pressures and their phabricator activity suggests they have limited time to contribute at present.
> > >
> > > As such code reviews for new features seem to sit unreviewed  and unaccepted and yet the subscribers list and tokens, the pings and "whats blocking this" comments would suggest that revisions such as these are wanted but sit for weeks and months with no movement.
> > >
> > > It feels like we need more people reviewing code and approving code changes otherwise these revisions bit rot or simply do not get into the tool. It definitely will disenfranchise new developers from contributing, if it takes months/years to get anything committed. But a shallow review by someone with at least a little experience of this code is surely better than no review at all. As such I'd like to volunteer to help review some of these items.
> >
> >
> > I don't think shallow reviews before having the architectural questions tied down would help a lot, as they in my experience create more opposition of change later on.
> >
> > I fully agree that the state we're in is bad, and I'm truly sorry for us to have arrived here.
> >
> > > Not all areas of clang have this same problem, I see areas of code which sometimes don't even get a review, where buddies review each others code very quickly, or where the time from creation to commit can be measured in hours or days, not weeks or months. I'm unclear what makes commits here take so long and good improvements lost. As such I don't think we get people contributing to fixing bugs either because the time taken to get even a bug landed seems quite great.
> >
> > One of the differences is whether there is ahead-of-code-review-time agreement on the direction, and whether the code fits in well from an architectural point of view.
> >  In clang-format we specifically do not want to support all possible features, but carefully weigh the upside of features against maintenance cost. Given that formatting is a prototypical bikeshed situation, I know that this is often subjective and thus really frustrating, and again, I'm truly sorry for it, but I don't have a good solution :(
> >
> > > From what I understand as long as changes meet the guidelines set out about adding new options, changes have tests and have proper documentation then we should be free to approve and land those changes, The code owners have been given time like the rest of us to review and in the event they don't like the commit after the fact are free to submit a revision later or ask for further modifications. (make sure you check the done message on each of their review comments as you address them and ensure you leave a comment if you are not going to do the suggested change and why)
> >
> > I think there's consensus between folks responsible for clang-format that we do specifically not want all features. The question is not only whether the code follows guidelines and is documented and tested. All code incurs cost and that has to be weighted carefully against the benefits.
> >
> > > As developers if we submit a revision we need to be prepared to look after that change and address and defects that come in, we don't want people driving by and dropping a patch that the code owners have to maintain, so with committing come responsibility.
> > > 
> > > I've personally been running with this patch in my clang format since @VelocityRa  took it over, and from what I can tell it works well, I too would like to see this landed.
> > > 
> > > With that said @VelocityRa and this comment, I would ask that you add something to the docs/ReleaseNotes.rst to show the addition of the new option to clang-format, and if you make that change, rebase (I doubt anything else was accepted in the meantime!) and send an updated revision I will add a LGTM and accept the review (for what that is worth), this will give @djasper  and @klimek  time to object to my proposal here.
> > > 
> > > Assuming others are in agreement, lets do something to move at least this forward.
>
>
> Thank you for responsing.. It seems no one is is able to approve code here without your or @djasper's  input, of course we understand that is the ideal situation because you wrote this in the first place, and your ability to understand the code is far superior than others. But i'm sure you time is precious, how can the rest of us grow to help out?
>
> I've heard the mantra before about "the cost of development", I'm not sure I understand why the "unit tests" aren't enough to prevent new styles breaking old capability, it feels like we don't have confidence that adding more won't break what is there already, and that is a lack of confidence that normally comes when code doesn't have enough tests, so why is clang-format so special that we can't cope with future additions? when the rest of clang (& C++ for that matter) is changing so rapidly underneath?

I don't think it's particularly different? The main difference is that it's much easier to come up with a change to clang-format that you really really want than to clang, because clang has a standard and clang-format invites contributions that are a matter of taste, which quickly leads to very heated arguments over things that one side might consider diminishing returns.

> To learn a code base in order to be able to help and spread that development cost, you need to work in that code base, that means adding things, fixing things, reviewing things. approving things... are you happy for others to contribute?

Yes, we are, and as you said, it needs a set of patches to learn the code base enough to contribute, to build up trust. This is easiest when the code base is currently under active development, but many people consider clang-format "good enough" minus bug fixes / new languages (like the C# patch, which I haven't looked at).

Are you by chance going to be at the Euro LLVM dev meeting? If so, I'd be happy to sit together with you to work on / talk through patches.

Overall, note that I don't see big architectural problems with this patch so far - it took me a while to realize it was re-written, but I believe architecturally it is fine and needs a good pass on the technical details before going in.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D28462/new/

https://reviews.llvm.org/D28462