[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke
https://bugzilla.redhat.com/show_bug.cgi?id=1564149 --- Comment #29 from Nigel Babu --- Um, so I've been accepting the pull requests that come in more or less mostly so we can see what the output looks like. I'm going to wait for 24h for each of the remaining pull requests for comments. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=un1MZubORN&a=cc_unsubscribe ___ Gluster-infra mailing list Gluster-infra@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke
https://bugzilla.redhat.com/show_bug.cgi?id=1564149
--- Comment #28 from Xavi Hernandez ---
(In reply to Aravinda VK from comment #27)
> I prefer Right(`AlignEscapedNewlines=Right`), since Patch will have minimum
> number of lines change on modification. For example,
>
> #define gf_log(dom, levl, fmt...) do { \
> FMT_WARN (fmt); \
> _gf_log (dom, __FILE__, __FUNCTION__, __LINE__, \
> levl, ##fmt); \
> } while (0)
>
> If longest line is changed then patch diff shows all of them as changed
> lines in case of `AlignEscapedNewLines=Left`
That makes sense. It's ok for me.
--
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug
https://bugzilla.redhat.com/token.cgi?t=CsBYbRgu0h&a=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke
https://bugzilla.redhat.com/show_bug.cgi?id=1564149
--- Comment #27 from Aravinda VK ---
(In reply to Xavi Hernandez from comment #19)
> I made a mistake on one of the proposed values:
>
> AlignEscapedNewLinesLeft=false
>
> What I wanted to propose was to set it to 'true' (which matches the
> explanation I gave).
I prefer Right(`AlignEscapedNewlines=Right`), since Patch will have minimum
number of lines change on modification. For example,
#define gf_log(dom, levl, fmt...) do { \
FMT_WARN (fmt); \
_gf_log (dom, __FILE__, __FUNCTION__, __LINE__, \
levl, ##fmt); \
} while (0)
If longest line is changed then patch diff shows all of them as changed lines
in case of `AlignEscapedNewLines=Left`
--
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug
https://bugzilla.redhat.com/token.cgi?t=eZWnKhPOYv&a=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke
https://bugzilla.redhat.com/show_bug.cgi?id=1564149 Aravinda VK changed: What|Removed |Added CC||avish...@redhat.com --- Comment #26 from Aravinda VK --- (In reply to Xavi Hernandez from comment #23) > The change will also include some of the controversial options: > > IndentCaseLabels=true > SpaceBeforeParens=ControlStatements +1 for only ControlStatements > > What should we do with them ? -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=2DpqqoBuZi&a=cc_unsubscribe ___ Gluster-infra mailing list Gluster-infra@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke
https://bugzilla.redhat.com/show_bug.cgi?id=1564149 --- Comment #25 from Xavi Hernandez --- I don't want to start any battle. It was requested to create pull requests with the proposed changes and that's what I've done. I've not hidden that some of the changes are debatable so that everyone can give his point of view. If they are not accepted it's ok for me. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=UQALtNHVM8&a=cc_unsubscribe ___ Gluster-infra mailing list Gluster-infra@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke
https://bugzilla.redhat.com/show_bug.cgi?id=1564149 --- Comment #24 from Jeff Darcy --- Personally I'm willing to go with the flow on those. I've laid out my reasoning, other people seem less inclined to compromise, I think we all have better things to do than turn it into a pitched battle. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=d6z8a0QlxO&a=cc_unsubscribe ___ Gluster-infra mailing list Gluster-infra@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke
https://bugzilla.redhat.com/show_bug.cgi?id=1564149 --- Comment #23 from Xavi Hernandez --- The change will also include some of the controversial options: IndentCaseLabels=true SpaceBeforeParens=ControlStatements What should we do with them ? -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=vxXuKAwVH5&a=cc_unsubscribe ___ Gluster-infra mailing list Gluster-infra@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke
https://bugzilla.redhat.com/show_bug.cgi?id=1564149
--- Comment #22 from Xavi Hernandez ---
Some issues I've seen:
* Some initializations are formatted like this:
char gfid_local[GF_UUID_BUF_SIZE] = {
0,
};
I've tried to change AllowShortBlocksOnASingleLine=true, but no change here.
* Braces after functions are not put into a new line. It seems that
BreakBeforeBraces=Attach has precedence to BraceWrapping.AfterFunction=true.
* AlignConsecutiveAssignments=true aligns some assignments inside a function
(not only variables initialization at the beginning). I think this is not
necessary.
* Some assignments are rendered like this:
meta_dst->size =
hton64(ntoh64(meta_dst->size) + ntoh64(meta_src->size));
meta_dst->file_count =
hton64(ntoh64(meta_dst->file_count) + ntoh64(meta_src->file_count));
I think we should increase PenaltyBreakAssignment to prevent this. I've chosen
a random value (200) and this is the result:
meta_dst->size = hton64(ntoh64(meta_dst->size) +
ntoh64(meta_src->size));
meta_dst->file_count = hton64(ntoh64(meta_dst->file_count) +
ntoh64(meta_src->file_count));
I'll create a new pull request with these changes.
--
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug
https://bugzilla.redhat.com/token.cgi?t=1tnUHvZJiN&a=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke
https://bugzilla.redhat.com/show_bug.cgi?id=1564149 --- Comment #21 from Jeff Darcy --- The feedback so far has been strongly positive, but one issue has been raised. As Xavi mentioned, the PointerAlignment doesn't seem to work properly on lines affected by AlignConsecutive*. AFAICT it's trying to center the asterisk, perhaps using PointerAlignment to decide left/right bias in case of a tie, but still leaving undesirable spaces between the asterisk and the variable name. Awkward. If these options are incompatible, which do *we* believe should take precedence? -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=gJdtESgCqG&a=cc_unsubscribe ___ Gluster-infra mailing list Gluster-infra@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke
https://bugzilla.redhat.com/show_bug.cgi?id=1564149 --- Comment #20 from Jeff Darcy --- FWIW, I've posted a sample of the result for my team at Facebook, and will forward any feedback. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=MsYXD0OHBD&a=cc_unsubscribe ___ Gluster-infra mailing list Gluster-infra@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1570161] New mailing list: t...@gluster.org
https://bugzilla.redhat.com/show_bug.cgi?id=1570161 --- Comment #1 from Amye Scavarda --- Editing to add: Archives are private, not that the list should be invisible. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=qjwxz2YgpF&a=cc_unsubscribe ___ Gluster-infra mailing list Gluster-infra@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-infra
[Gluster-infra] [Bug 1570161] New mailing list: t...@gluster.org
https://bugzilla.redhat.com/show_bug.cgi?id=1570161 Amye Scavarda changed: What|Removed |Added CC||gluster-infra@gluster.org Component|unclassified|project-infrastructure -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=tvcYenVybJ&a=cc_unsubscribe ___ Gluster-infra mailing list Gluster-infra@gluster.org http://lists.gluster.org/mailman/listinfo/gluster-infra
