Recently, fixeria brought up that it would be better to not align wrapped lines with an opening brace, but rather have a constant indent for wrapped lines.
I'd like to ask for the general opinion / kernel style compliance. What indenting is acceptable?
For a long time my opinion was that opening brace alignment is better for readability, but my opinion has shifted. I now find it more desirable to keep a constant indent.
Consider:
unsigned int my_function_signature(int a_very_long_parameter, int wrapped_line, int another_line);
If a patch modifies the return value or the name, the patch either touches three lines or leaves inconsistent indenting behind:
int my_func(int a_very_long_parameter, int wrapped_line, int another_line);
Also, a very long function name wastes a lot of usable linewidth for indent for all arguments.
I've submitted a lot of patches over the years that touch more lines than necessary, and left quite a few inconsistent indents behind, because an automatic find-replace doesn't fix subsequent indenting.
I'm currently editing a lot of the osmo-msc sources and changing my mind about API, so this line indenting due to renames issue comes up. I thought I could pick a constant indent now while I'm at it.
If we had a single tab as indent, the function signatures' indenting would be independent from the length of the name, and a find-replace leaves behind consistent indenting / no whitespace changes needed for renames:
unsigned int my_function_signature(int a_very_long_parameter, int wrapped_line, int another_line);
int my_func(int a_very_long_parameter, int wrapped_line, int another_line);
I finally understood the vim cindent options to achieve this:
:se cino=(1s,u1s (Forgive me if this is a bit too vim specific for your taste; I definitely want vim to automatically give me the desired indenting.)
My problem with that is that it also affects if- and for- statements: 'if' and 'for' will never change their name, so aligning with opening-brace is unlikely to ever cause unnecessary whitespace change. What was:
// vim: cino=>1s,e0,f0,{0,}0,=1s,t0,c3,+1s,(0,u0,/0,:0
int my_func(int a_very_long_parameter, int wrapped_line, int another_line) { if (a_very_long_parameter == 3 || a_very_long_parameter < 0 || wrapped_line == 7 || (one_level_deeper && (one_level_deeper || foo))) return 5; }
Now becomes:
// vim: cino=>1s,e0,f0,{0,}0,=1s,t0,c3,+1s,(1s,u1s,/0,:0
int my_func(int a_very_long_parameter, int wrapped_line, int another_line) { if (a_very_long_parameter == 3 || a_very_long_parameter < 0 || wrapped_line == 7 || (one_level_deeper && (one_level_deeper || foo))) return 5; }
(The cino=k0 parameter does control 'if', 'for' and 'while' separately, but there is no way to tell it to brace-align while the '(' option is set to '(1s', stupid quirk in cinoptions.)
IMHO aligning with opening brace improves readability of the nested conditions and uses more of the line width. But I could get used to that, I guess. I think I've seen this used a lot in Osmocom, too.
Another thing to consider:
if (a_very_long_parameter == 3 || a_very_long_parameter < 0 || wrapped_line == 7) return has_same_indent_as_condition(" :/ ");
I could use two tabs for in-brace indenting, but I haven't seen that anywhere before, and it makes wrapped conditions waste a lot of line width:
// vim: cino=>1s,e0,f0,{0,}0,=1s,t0,c3,+1s,(2s,u1s,/0,:0
int my_function_signature(int a_very_long_parameter, int wrapped_line, int another_line) { if (a_very_long_parameter == 3 || a_very_long_parameter < 0 || wrapped_line == 7 || (one_level_deeper && (one_level_deeper || foo))) return 5;
if (a_very_long_parameter == 3 || a_very_long_parameter < 0 || wrapped_line == 7) return 5; }
I think I'll adopt fixed single-tab indenting in my patches from now on: :se cino=>1s,e0,f0,{0,}0,=1s,t0,c3,+1s,(1s,u1s,k0,/0,:0
Any (strong) opinions on / best practices for this? Which one is more consistent with linux kernel style?
Thanks!
~N
Hi Neels,
On Wed, Feb 06, 2019 at 12:29:19AM +0100, Neels Hofmeyr wrote:
I'd like to ask for the general opinion / kernel style compliance. What indenting is acceptable?
My personal opinion is that the indent should match the opening brace, or at least the nearest tab very close to it.
In the kernel (thought I don't spend much time in it anymore) I haven't seen anything else but opening brace.
I would appreciate if we could keep it that way and focus our energy on actual implementation work. Of course, if you guys want to discuss it, you are free. The above is just my point of view.
Regards, Harald
Hi Neels,
Recently, fixeria brought up that it would be better to not align wrapped lines with an opening brace [...]
as you've already mentioned, I am not a big fun of such alignment. IMHO, this coding style neither improves nor decreases the total readability - it's just the matter of taste.
Why don't we align variable assignments then? Like this:
struct msgb *msg = (struct msgb *) data; unsigned long msg_len = msgb_length(msg); uint8_t *ptr = msg->data + 3;
Yes, it's aligned, but is it easier to read? Thanks to the code highlighting, I don't see significant readability changes. But, if we add a new variable with longer length, we would have to realign all surrounding definitions. So, as a result we have ~same readability, but additional responsibility?
At the same time, I like the alignment we use for value_string definitions, and for osmo_fsm definitions, because it actually changes the readability in a better way, and doesn't require us to realign everything in the most cases.
My problem with that is that it also affects if- and for- statements: 'if' and 'for' will never change their name, so aligning with opening-brace is unlikely to ever cause unnecessary whitespace change.
Agree, for both mentioned statements we really need to distinguish between their body and their definition, otherwise e.g. a wrapped part of some condition may look like a part of the code to be executed. But, there is no such problem for function definitions.
Any (strong) opinions on / best practices for this?
Sure, we should focus on actual implementation work, but since this question arises from time to time during this implementation work, let's discuss it once and for all.
I would like to propose some of my ideas:
1. let's don't enforce the "opening brace alignment" during code review; 1.a. if a new file is to be introduced, the alignment style is up to the author; 1.b. the alignment style should be consistent in the whole file, 1.b. ... but if the function name is too long, let's allow a single tab;
2. let's avoid the "opening brace alignment" for the function calls, e.g.
vty_out(vty, "Alignment makes out code great ...\n");
let's just use a single tab.
3. we should neither submit patches like "align function definitions", nor "use a single space for alignment" to the existing code; 3.a. excepting the cases, when arguments left misaligned after the function name change and look ugly after that;
With best regards, Vadim Yanitskiy.
On Wed, Feb 06, 2019 at 09:44:39PM +0700, Vadim Yanitskiy wrote:
- let's don't enforce the "opening brace alignment" during code review;
If we decided on this, then the next one would be a problem:
1.a. if a new file is to be introduced, the alignment style is up to the author;
I will not change my cinoptions according to .c file, I will pick one style and always use that. The only way I would accept to adopt single-tab indent would be to allow it anywhere, even when mixing style in files; i.e. allow both.
I still agree that a single-tab indenting is better for using the line width and for having less whitespace changes in patches. I also have a number of other preferences in coding style.
But in coding style questions, I'm taking Harald's and Holger's opinions as the benchmark and last word on things... So I guess it was worth a try, but I'll just continue as before, then.
~N
On 6. Feb 2019, at 12:34, Harald Welte laforge@gnumonks.org wrote:
I would appreciate if we could keep it that way and focus our energy on actual implementation work. Of course, if you guys want to discuss it, you are free. The above is just my point of view.
I have been using clang-format for a while. It is quite relieving to just type and let the tool format the code according to the agreed style. We can use the kernel .clang-format and modify it for our changes.
This can be integrated into the jenkins tests to fail verification if clang-format would introduce a diff.
holger
On 2/7/19 12:05 PM, Holger Freyther wrote> I have been using clang-format for a while. It is quite relieving to just type and let the tool format the code according to the agreed style. We can use the kernel .clang-format and modify it for our changes.
This can be integrated into the jenkins tests to fail verification if clang-format would introduce a diff.
This would be my favorite approach, especially with enforcing through jenkins.
Oliver
Hi.
In general though, I'm pretty happy with the current alignment we use right now because my editor automatically does that for me. Yes, some patches changing only some of the parameters might introduce inconsistencies but it doesn't seem like a big deal to me. Either relaxing patch review to allow realignment in the same patch as functional change or having separate cosmetic patch would fix that.
So personally I don't see any need to change anything right now.
07.02.19 12:05, Holger Freyther пишет:
I have been using clang-format for a while. It is quite relieving to just type and let the tool format the code according to the agreed style. We can use the kernel .clang-format and modify it for our changes.
This can be integrated into the jenkins tests to fail verification if clang-format would introduce a diff.
holger
Wouldn't we need to fix formatting of all the files first?
Otherwise patches fixing current "incorrect" formatting to whatever we agree on would fail the tests.
Hi Holger,
I'm personally not convinced any related tools would improve my personal workflow, but if it helps others, I'm of course happy about related suggestions.
On Thu, Feb 07, 2019 at 11:05:23AM +0000, Holger Freyther wrote:
This can be integrated into the jenkins tests to fail verification if clang-format would introduce a diff.
wireshark has somwthing like a pre-commit hook in their repository that prevents you from even pushing changes that don't pass validation (such as non-terminated value_string arrays). I've never studied it in detail how they do it, but it works out of the box after cloning the repository. I don't recall having to manually configure something to get this.
Regards, Harald
On Fri, Feb 08, 2019 at 09:06:04AM +0100, Harald Welte wrote:
wireshark has somwthing like a pre-commit hook in their repository that prevents you from even pushing changes that don't pass validation (such as non-terminated value_string arrays).
probably a pre-push hook in the upstream repos. Things like coding style / value strings validation would be ok for that. (obviously not full CI runs that take minutes and longer)
Also I wonder whether gerrit's java git stack allows having a pre-push hook.
I kind of like the idea of using a code formatting linter, it (hopefully) completely cuts out all discussion.
I remember some blog post about that ... don't know where. The point was, any and all of these little things should be enforced by CI, it creates a clear situation and cuts out endless discussions.
Would be trivial to run a code reformatting command before submitting to gerrit.
However, the hard part would be to get a reliable code formatter that doesn't do nonsense. For example, I think dexter once used such tool that produced a bit of nonsense indenting / formatting every now and then.
But... I'm heading back to inter-MSC handover now.
~N
Hi,
I used to work in an environment where we used uncrustify [1] to check formatting during CI time (github + jenkins iirc).
In general it worked quite well but sometimes indeed it did some unexpected stuff.
So I'd say I'm not for or against using this kind of tooling during CI to check formatting. It has good stuff (avoid adding whitespace artifacts and bad formatting), but on the other side it has issues (more time spent on formatting, sometimes it adds its own formatting artifacts).
Furthermore, that means probably we need to run it once over all the code base and submit patches to fix it so we can then use it as a base to check the changes.
[1] http://uncrustify.sourceforge.net/
-
Harald Welte -
Holger Freyther -
Max -
Neels Hofmeyr -
Oliver Smith -
Pau Espin Pedrol -
Vadim Yanitskiy