After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 744521 - Programming guidelines: C coding style page: batch of various improvements
Programming guidelines: C coding style page: batch of various improvements
Status: RESOLVED FIXED
Product: gnome-devel-docs
Classification: Applications
Component: programming-guidelines
3.14.x
Other Linux
: Normal enhancement
: ---
Assigned To: GNOME Devel docs: Programming Guidelines maintainer(s)
gnome-devel-docs maintainers
Depends on:
Blocks:
 
 
Reported: 2015-02-14 12:28 UTC by Sébastien Wilmet
Modified: 2015-02-14 20:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
programming-guidelines: Linux Kernel indentation style is with tabs (1.75 KB, patch)
2015-02-14 12:32 UTC, Sébastien Wilmet
committed Details | Review
programming-guidelines: fix code using the Linux Kernel style (6.79 KB, patch)
2015-02-14 12:32 UTC, Sébastien Wilmet
committed Details | Review
programming-guidelines: fix GNU code example and re-order items (2.34 KB, patch)
2015-02-14 12:32 UTC, Sébastien Wilmet
committed Details | Review
programming-guidelines: long conditions are hard to read (1.37 KB, patch)
2015-02-14 12:33 UTC, Sébastien Wilmet
committed Details | Review
programming-guidelines: don't recommend to replace tabs by 8 spaces (1.36 KB, patch)
2015-02-14 19:04 UTC, Sébastien Wilmet
rejected Details | Review

Description Sébastien Wilmet 2015-02-14 12:28:39 UTC
See the attached patches (also in the wip/swilmet/various-fixes branch).
Comment 1 Sébastien Wilmet 2015-02-14 12:32:43 UTC
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
Comment 2 Sébastien Wilmet 2015-02-14 12:32:50 UTC
Created attachment 296820 [details] [review]
programming-guidelines: fix code using the Linux Kernel style

Replace 8 spaces by tabs.
Comment 3 Sébastien Wilmet 2015-02-14 12:32:58 UTC
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.
Comment 4 Sébastien Wilmet 2015-02-14 12:33:21 UTC
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.
Comment 5 David King 2015-02-14 14:41:35 UTC
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.
Comment 6 David King 2015-02-14 14:42:06 UTC
Review of attachment 296820 [details] [review]:

OK.
Comment 7 David King 2015-02-14 14:43:11 UTC
Review of attachment 296821 [details] [review]:

OK.
Comment 8 David King 2015-02-14 14:43:41 UTC
Review of attachment 296822 [details] [review]:

Seems fine.
Comment 9 Sébastien Wilmet 2015-02-14 15:02:17 UTC
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).
Comment 10 David King 2015-02-14 15:10:54 UTC
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.
Comment 11 Sébastien Wilmet 2015-02-14 15:30:01 UTC
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.
Comment 12 David King 2015-02-14 15:37:57 UTC
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.
Comment 13 Sébastien Wilmet 2015-02-14 17:00:52 UTC
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
Comment 14 David King 2015-02-14 17:53:22 UTC
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 15 Sébastien Wilmet 2015-02-14 18:53:43 UTC
Comment on attachment 296819 [details] [review]
programming-guidelines: Linux Kernel indentation style is with tabs

Adapted as requested and pushed.
Comment 16 Sébastien Wilmet 2015-02-14 19:04:52 UTC
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.
Comment 17 David King 2015-02-14 19:26:43 UTC
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.
Comment 18 Sébastien Wilmet 2015-02-14 20:04:23 UTC
That's clearer.