GNOME Bugzilla – Bug 744521
Programming guidelines: C coding style page: batch of various improvements
Last modified: 2015-02-14 20:04:23 UTC
See the attached patches (also in the wip/swilmet/various-fixes branch).
Created attachment 296819 [details] [review] programming-guidelines: Linux Kernel indentation style is with tabs The reference document is a bit confusing: https://www.kernel.org/doc/Documentation/CodingStyle But the indentation is with tabs, with a *length* of 8 characters. You can check the Linux kernel code, here is for example a random file: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/kernel/async.c Or in Nautilus: https://git.gnome.org/browse/nautilus/tree/src/nautilus-application.c
Created attachment 296820 [details] [review] programming-guidelines: fix code using the Linux Kernel style Replace 8 spaces by tabs.
Created attachment 296821 [details] [review] programming-guidelines: fix GNU code example and re-order items 1. move the GNU-specific item to the top 2. fix GNU example in a later example If the GNU-specific item is explained after (2), one can wonder why the Linux and GNU styles are different for the same example.
Created attachment 296822 [details] [review] programming-guidelines: long conditions are hard to read The introduction of the page explains that code maintainability is important, so having a note about a programming best practice is useful I think. The code example was more to explain the coding style, but with the added note, we explain that we are aware that such long conditions are not a good example of clean code.
Review of attachment 296819 [details] [review]: ::: programming-guidelines/C/c-coding-style.page @@ -179,3 @@ Instead, set the <em>indentation size</em> as appropriate for - the code you are editing. You may even be able to tell your - editor to automatically convert all tabs to 8 spaces, so that Leave this bit in.
Review of attachment 296820 [details] [review]: OK.
Review of attachment 296821 [details] [review]: OK.
Review of attachment 296822 [details] [review]: Seems fine.
Thanks for the quick review. (In reply to David King from comment #5) > Review of attachment 296819 [details] [review] [review]: > > ::: programming-guidelines/C/c-coding-style.page > @@ -179,3 @@ > Instead, set the <em>indentation size</em> as appropriate for > - the code you are editing. You may even be able to tell your > - editor to automatically convert all tabs to 8 spaces, so that > > Leave this bit in. Why is it useful to convert all tabs to 8 spaces? The linux kernel style is with tabs, not spaces. We can maybe change the sentence for 2 spaces (for the GNU style).
I don't think that we should be recommending tab at all for new code. Tabs are ambiguous because of the ability to change the visible length, and converting to spaces avoid that problem. Only old code with a history of tabs should use tabs, and the advice is sound.
8-spaces indentation is not the Linux kernel convention. The important thing with a convention is to be consistent, also across projects. If each project adds exceptions to a certain convention, it's a nightmare for someone willing to contribute. So if even the GNOME programming guidelines is ambiguous about the conventions, it'll not improve the situation. The c-coding-style page already explains (just above, in the same paragraph): "Do not ever change the size of tabs in your editor; leave them as 8 spaces. Changing the size of tabs means that code that you didn’t write yourself will be perpetually misaligned." So configuring the text editor to replace tabs by spaces should only be done for the GNU style, i.e. 2 spaces.
That paragraph is not specific to Linux kernel conventions, and is generic advice, which I fully agree with. You can reword to be more clear, but it should not be removed.
You either configure tab to insert 2 spaces if you use the GNU coding style, or you configure tab to insert a real tab if you use the Linux coding style. 2/3 of the c-coding-style page talks about the GNU style and the Linux style, with that paragraph in the middle. You cannot ignore the surrounding context! Writing an advice there to configure the text editor to insert 8 spaces when a tab is pressed is just stupid… Anyway, this discussion is going a bit too bikeshed just for a small sentence. I thought the change was obvious enough… But you repeatedly use authoritative arguments ("which I fully agree with", "I do not think that" [1]), which is not how free software work… So to be honest it's a bit painful to contribute. My original intent is to fully read the programming guidelines and write patches where I think it's needed, and I try to give good arguments (except when I think the change in itself is obvious, but in that case I can explain in more details if needed). [1] https://bugzilla.gnome.org/show_bug.cgi?id=744462#c1
It seems that there has been a misunderstanding, and for that I am sorry. I am happy to review patches, and I have suggested some improvements, but I will not accept a change that I am not happy with.
Comment on attachment 296819 [details] [review] programming-guidelines: Linux Kernel indentation style is with tabs Adapted as requested and pushed.
Created attachment 296841 [details] [review] programming-guidelines: don't recommend to replace tabs by 8 spaces The Linux coding style uses tabs, not spaces. The GNU style uses 2 spaces, not 8. So this sentence has nothing to do in the c-coding-style page. The exact point of a convention is to not create exceptions to those conventions, otherwise every project will have a different coding style and it won't improve the situation in GNOME.
Review of attachment 296841 [details] [review]: I instead pushed a clarification of the sentence to master as commit e2512a428dbbef5a9cd774f446c027f6246e869c. There is still room for improvement regarding mixing of tabs and spaces.
That's clearer.