You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Leif Hedstrom <zw...@apache.org> on 2016/06/16 21:04:33 UTC
[PROPOSAL] One more clang-format change for 7.0.0
Hi all,
James and I would like to make this small, but powerful, change to clang-format:
-AlignConsecutiveAssignments: false
+AlignConsecutiveAssignments: true
This will try to align assignments such that the = signs line up. E.g.
enum {
XHEADER_X_CACHE_KEY = 0x0004u,
XHEADER_X_MILESTONES = 0x0008u,
XHEADER_X_CACHE = 0x0010u,
XHEADER_X_GENERATION = 0x0020u,
XHEADER_X_TXN_UUID = 0x0040u,
};
This is a pretty significant change (about 12k lines in 650 files), but I think it’s worth it. And again, since we have clang-format, getting your old branches up to this new indentation is easy, just temporarily copy over the new .clang-format, and re-run “make clang-format", and voila, you are up-to-date with master as far as formatting goes.
Please voice concerns with this now. To make the LTS release manageable and easy, we’d also back port this to the next RC1 for v6.2.0. I have spoken with He-Who-Shall-Make-All-LTS-Releases, and he’s +1 on this idea as well.
Cheers,
— leif
Re: [PROPOSAL] One more clang-format change for 7.0.0
Posted by Leif Hedstrom <zw...@apache.org>.
> On Jun 16, 2016, at 3:04 PM, Leif Hedstrom <zw...@apache.org> wrote:
>
> Hi all,
>
> James and I would like to make this small, but powerful, change to clang-format:
>
> -AlignConsecutiveAssignments: false
> +AlignConsecutiveAssignments: true
>
>
> This will try to align assignments such that the = signs line up. E.g.
>
> enum {
> XHEADER_X_CACHE_KEY = 0x0004u,
> XHEADER_X_MILESTONES = 0x0008u,
> XHEADER_X_CACHE = 0x0010u,
> XHEADER_X_GENERATION = 0x0020u,
> XHEADER_X_TXN_UUID = 0x0040u,
> };
sigh, and of course email doesn’t indent well… look at this instead:
http://pastebin.com/raw/rXVmLFH7 <http://pastebin.com/raw/rXVmLFH7>
— leif
Re: [PROPOSAL] One more clang-format change for 7.0.0
Posted by Leif Hedstrom <zw...@apache.org>.
> On Jun 21, 2016, at 2:06 PM, Phil Sorber <so...@apache.org> wrote:
>
> On Tue, Jun 21, 2016 at 2:31 PM James Peach <jp...@apache.org> wrote:
>
>>
>>> On Jun 21, 2016, at 12:19 PM, Phil Sorber <so...@apache.org> wrote:
>>>
>>> So I was +1 on this, but now looking at the full diff I am -0. I wanted
>> to
>>> get others take on this. It aligns *all* assignments not just declaration
>>> ones.
>>>
>>> For example:
>>> - c->vio.op = VIO::READ;
>>> + c->vio.op = VIO::READ;
>>> c->base_stat = cache_lookup_active_stat;
>>> CACHE_INCREMENT_DYN_STAT(c->base_stat + CACHE_STAT_ACTIVE);
>>> c->first_key = c->key = *key;
>>> - c->frag_type = type;
>>> - c->f.lookup = 1;
>>> - c->vol = vol;
>>> - c->last_collision = NULL;
>>> + c->frag_type = type;
>>> + c->f.lookup = 1;
>>> + c->vol = vol;
>>> + c->last_collision = NULL;
>>>
>>> This seems weird and distracting to me. It groups ones on consecutive
>> lines
>>> too. So you might have different spacing within one body of code because
>>> there are other non-assignment lines in between. It also doesn't appear
>> to
>>> align things like +=.
>>>
>>> - cstate->cache_vc = (TSVConn)edata;
>>> - cstate->write_vio = TSVConnWrite(cstate->net_vc, contp,
>>> cstate->resp_reader, INT64_MAX);
>>> + cstate->cache_vc = (TSVConn)edata;
>>> + cstate->write_vio = TSVConnWrite(cstate->net_vc, contp,
>>> cstate->resp_reader, INT64_MAX);
>>> cstate->total_bytes += TSIOBufferWrite(cstate->resp_buffer, error,
>>> sizeof(error) - 1);
>>>
>>> If everyone is still onboard then lets stick with it, but just wanted to
>>> talk this out.
>>
>> Yeh I agree that there are ugly cases, but on balance I think this made
>> the formatting cleaner. With some judicious blank lines to reset the
>> indentation and application of clang-format-off, can we improve the
>> distracting cases you found?
>>
>
> I think the distracting cases I found were due to reset indentation.
> Ignoring the augmented assignment won't be fixed either.
>
> I think if everyone is on board, I can make do. Just wanted to have this
> conversation.
Hmmm, I think I hate this now… It really makes shit looks super ugly in a bunch of cases, to the point where the ugliness outweighs the goodness.
Can / should we just undo this, and re-run clang-format again? I would hate to revert it, because that will cause chaos I think.
— Leif
Re: [PROPOSAL] One more clang-format change for 7.0.0
Posted by Phil Sorber <so...@apache.org>.
On Tue, Jun 21, 2016 at 2:31 PM James Peach <jp...@apache.org> wrote:
>
> > On Jun 21, 2016, at 12:19 PM, Phil Sorber <so...@apache.org> wrote:
> >
> > So I was +1 on this, but now looking at the full diff I am -0. I wanted
> to
> > get others take on this. It aligns *all* assignments not just declaration
> > ones.
> >
> > For example:
> > - c->vio.op = VIO::READ;
> > + c->vio.op = VIO::READ;
> > c->base_stat = cache_lookup_active_stat;
> > CACHE_INCREMENT_DYN_STAT(c->base_stat + CACHE_STAT_ACTIVE);
> > c->first_key = c->key = *key;
> > - c->frag_type = type;
> > - c->f.lookup = 1;
> > - c->vol = vol;
> > - c->last_collision = NULL;
> > + c->frag_type = type;
> > + c->f.lookup = 1;
> > + c->vol = vol;
> > + c->last_collision = NULL;
> >
> > This seems weird and distracting to me. It groups ones on consecutive
> lines
> > too. So you might have different spacing within one body of code because
> > there are other non-assignment lines in between. It also doesn't appear
> to
> > align things like +=.
> >
> > - cstate->cache_vc = (TSVConn)edata;
> > - cstate->write_vio = TSVConnWrite(cstate->net_vc, contp,
> > cstate->resp_reader, INT64_MAX);
> > + cstate->cache_vc = (TSVConn)edata;
> > + cstate->write_vio = TSVConnWrite(cstate->net_vc, contp,
> > cstate->resp_reader, INT64_MAX);
> > cstate->total_bytes += TSIOBufferWrite(cstate->resp_buffer, error,
> > sizeof(error) - 1);
> >
> > If everyone is still onboard then lets stick with it, but just wanted to
> > talk this out.
>
> Yeh I agree that there are ugly cases, but on balance I think this made
> the formatting cleaner. With some judicious blank lines to reset the
> indentation and application of clang-format-off, can we improve the
> distracting cases you found?
>
I think the distracting cases I found were due to reset indentation.
Ignoring the augmented assignment won't be fixed either.
I think if everyone is on board, I can make do. Just wanted to have this
conversation.
Thanks.
>
> >
> > Thanks.
> >
> > On Sun, Jun 19, 2016 at 9:17 PM Leif Hedstrom <zw...@apache.org> wrote:
> >
> >>
> >>> On Jun 17, 2016, at 9:46 AM, James Peach <jp...@apache.org> wrote:
> >>>
> >>>>
> >>>> On Jun 16, 2016, at 2:04 PM, Leif Hedstrom <zw...@apache.org> wrote:
> >>>>
> >>>> Hi all,
> >>>>
> >>>> James and I would like to make this small, but powerful, change to
> >> clang-format:
> >>>>
> >>>> -AlignConsecutiveAssignments: false
> >>>> +AlignConsecutiveAssignments: true
> >>>>
> >>>>
> >>>> This will try to align assignments such that the = signs line up. E.g.
> >>>>
> >>>> enum {
> >>>> XHEADER_X_CACHE_KEY = 0x0004u,
> >>>> XHEADER_X_MILESTONES = 0x0008u,
> >>>> XHEADER_X_CACHE = 0x0010u,
> >>>> XHEADER_X_GENERATION = 0x0020u,
> >>>> XHEADER_X_TXN_UUID = 0x0040u,
> >>>> };
> >>>
> >>> +1 from me.
> >>
> >>
> >>
> >> I have committed this. One side effect of this is that all PR’s likely
> >> will have to be updated with a new “make clang-format”.
> >>
> >> Cheers,
> >>
> >> — leif
> >>
> >> P.s
> >> One thing to be aware of, the alignment on the ‘=‘ resets if you have an
> >> empty line or a comment in the middle of e.g. an enum, or list of
> >> assignments in general.
>
>
Re: [PROPOSAL] One more clang-format change for 7.0.0
Posted by James Peach <jp...@apache.org>.
> On Jun 21, 2016, at 12:19 PM, Phil Sorber <so...@apache.org> wrote:
>
> So I was +1 on this, but now looking at the full diff I am -0. I wanted to
> get others take on this. It aligns *all* assignments not just declaration
> ones.
>
> For example:
> - c->vio.op = VIO::READ;
> + c->vio.op = VIO::READ;
> c->base_stat = cache_lookup_active_stat;
> CACHE_INCREMENT_DYN_STAT(c->base_stat + CACHE_STAT_ACTIVE);
> c->first_key = c->key = *key;
> - c->frag_type = type;
> - c->f.lookup = 1;
> - c->vol = vol;
> - c->last_collision = NULL;
> + c->frag_type = type;
> + c->f.lookup = 1;
> + c->vol = vol;
> + c->last_collision = NULL;
>
> This seems weird and distracting to me. It groups ones on consecutive lines
> too. So you might have different spacing within one body of code because
> there are other non-assignment lines in between. It also doesn't appear to
> align things like +=.
>
> - cstate->cache_vc = (TSVConn)edata;
> - cstate->write_vio = TSVConnWrite(cstate->net_vc, contp,
> cstate->resp_reader, INT64_MAX);
> + cstate->cache_vc = (TSVConn)edata;
> + cstate->write_vio = TSVConnWrite(cstate->net_vc, contp,
> cstate->resp_reader, INT64_MAX);
> cstate->total_bytes += TSIOBufferWrite(cstate->resp_buffer, error,
> sizeof(error) - 1);
>
> If everyone is still onboard then lets stick with it, but just wanted to
> talk this out.
Yeh I agree that there are ugly cases, but on balance I think this made the formatting cleaner. With some judicious blank lines to reset the indentation and application of clang-format-off, can we improve the distracting cases you found?
>
> Thanks.
>
> On Sun, Jun 19, 2016 at 9:17 PM Leif Hedstrom <zw...@apache.org> wrote:
>
>>
>>> On Jun 17, 2016, at 9:46 AM, James Peach <jp...@apache.org> wrote:
>>>
>>>>
>>>> On Jun 16, 2016, at 2:04 PM, Leif Hedstrom <zw...@apache.org> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> James and I would like to make this small, but powerful, change to
>> clang-format:
>>>>
>>>> -AlignConsecutiveAssignments: false
>>>> +AlignConsecutiveAssignments: true
>>>>
>>>>
>>>> This will try to align assignments such that the = signs line up. E.g.
>>>>
>>>> enum {
>>>> XHEADER_X_CACHE_KEY = 0x0004u,
>>>> XHEADER_X_MILESTONES = 0x0008u,
>>>> XHEADER_X_CACHE = 0x0010u,
>>>> XHEADER_X_GENERATION = 0x0020u,
>>>> XHEADER_X_TXN_UUID = 0x0040u,
>>>> };
>>>
>>> +1 from me.
>>
>>
>>
>> I have committed this. One side effect of this is that all PR’s likely
>> will have to be updated with a new “make clang-format”.
>>
>> Cheers,
>>
>> — leif
>>
>> P.s
>> One thing to be aware of, the alignment on the ‘=‘ resets if you have an
>> empty line or a comment in the middle of e.g. an enum, or list of
>> assignments in general.
Re: [PROPOSAL] One more clang-format change for 7.0.0
Posted by Phil Sorber <so...@apache.org>.
So I was +1 on this, but now looking at the full diff I am -0. I wanted to
get others take on this. It aligns *all* assignments not just declaration
ones.
For example:
- c->vio.op = VIO::READ;
+ c->vio.op = VIO::READ;
c->base_stat = cache_lookup_active_stat;
CACHE_INCREMENT_DYN_STAT(c->base_stat + CACHE_STAT_ACTIVE);
c->first_key = c->key = *key;
- c->frag_type = type;
- c->f.lookup = 1;
- c->vol = vol;
- c->last_collision = NULL;
+ c->frag_type = type;
+ c->f.lookup = 1;
+ c->vol = vol;
+ c->last_collision = NULL;
This seems weird and distracting to me. It groups ones on consecutive lines
too. So you might have different spacing within one body of code because
there are other non-assignment lines in between. It also doesn't appear to
align things like +=.
- cstate->cache_vc = (TSVConn)edata;
- cstate->write_vio = TSVConnWrite(cstate->net_vc, contp,
cstate->resp_reader, INT64_MAX);
+ cstate->cache_vc = (TSVConn)edata;
+ cstate->write_vio = TSVConnWrite(cstate->net_vc, contp,
cstate->resp_reader, INT64_MAX);
cstate->total_bytes += TSIOBufferWrite(cstate->resp_buffer, error,
sizeof(error) - 1);
If everyone is still onboard then lets stick with it, but just wanted to
talk this out.
Thanks.
On Sun, Jun 19, 2016 at 9:17 PM Leif Hedstrom <zw...@apache.org> wrote:
>
> > On Jun 17, 2016, at 9:46 AM, James Peach <jp...@apache.org> wrote:
> >
> >>
> >> On Jun 16, 2016, at 2:04 PM, Leif Hedstrom <zw...@apache.org> wrote:
> >>
> >> Hi all,
> >>
> >> James and I would like to make this small, but powerful, change to
> clang-format:
> >>
> >> -AlignConsecutiveAssignments: false
> >> +AlignConsecutiveAssignments: true
> >>
> >>
> >> This will try to align assignments such that the = signs line up. E.g.
> >>
> >> enum {
> >> XHEADER_X_CACHE_KEY = 0x0004u,
> >> XHEADER_X_MILESTONES = 0x0008u,
> >> XHEADER_X_CACHE = 0x0010u,
> >> XHEADER_X_GENERATION = 0x0020u,
> >> XHEADER_X_TXN_UUID = 0x0040u,
> >> };
> >
> > +1 from me.
>
>
>
> I have committed this. One side effect of this is that all PR’s likely
> will have to be updated with a new “make clang-format”.
>
> Cheers,
>
> — leif
>
> P.s
> One thing to be aware of, the alignment on the ‘=‘ resets if you have an
> empty line or a comment in the middle of e.g. an enum, or list of
> assignments in general.
Re: [PROPOSAL] One more clang-format change for 7.0.0
Posted by Leif Hedstrom <zw...@apache.org>.
> On Jun 17, 2016, at 9:46 AM, James Peach <jp...@apache.org> wrote:
>
>>
>> On Jun 16, 2016, at 2:04 PM, Leif Hedstrom <zw...@apache.org> wrote:
>>
>> Hi all,
>>
>> James and I would like to make this small, but powerful, change to clang-format:
>>
>> -AlignConsecutiveAssignments: false
>> +AlignConsecutiveAssignments: true
>>
>>
>> This will try to align assignments such that the = signs line up. E.g.
>>
>> enum {
>> XHEADER_X_CACHE_KEY = 0x0004u,
>> XHEADER_X_MILESTONES = 0x0008u,
>> XHEADER_X_CACHE = 0x0010u,
>> XHEADER_X_GENERATION = 0x0020u,
>> XHEADER_X_TXN_UUID = 0x0040u,
>> };
>
> +1 from me.
I have committed this. One side effect of this is that all PR’s likely will have to be updated with a new “make clang-format”.
Cheers,
— leif
P.s
One thing to be aware of, the alignment on the ‘=‘ resets if you have an empty line or a comment in the middle of e.g. an enum, or list of assignments in general.
Re: [PROPOSAL] One more clang-format change for 7.0.0
Posted by James Peach <jp...@apache.org>.
> On Jun 16, 2016, at 2:04 PM, Leif Hedstrom <zw...@apache.org> wrote:
>
> Hi all,
>
> James and I would like to make this small, but powerful, change to clang-format:
>
> -AlignConsecutiveAssignments: false
> +AlignConsecutiveAssignments: true
>
>
> This will try to align assignments such that the = signs line up. E.g.
>
> enum {
> XHEADER_X_CACHE_KEY = 0x0004u,
> XHEADER_X_MILESTONES = 0x0008u,
> XHEADER_X_CACHE = 0x0010u,
> XHEADER_X_GENERATION = 0x0020u,
> XHEADER_X_TXN_UUID = 0x0040u,
> };
+1 from me.