GNOME Bugzilla – Bug 376123
Programming Guidelines need updating
Last modified: 2015-02-11 10:25:32 UTC
The page here: http://developer.gnome.org/doc/guides/programming-guidelines/code-style.html lists the recommended coding style for GNOME C source code. "Use 8-space tabs for indentation." - This doesn't seem to be recommended anymore. Atleast many projects like GTK+ uses two spaces indentation. The page should also mention the recommended brace placement. { on the same line, { directly below or { below and indented. I also wish that that page could "more strongly" recommend (or demand) a coding style. It's a big PITA that many GNOME projects use totally different coding styles.
I guess it's not doable to enforce a coding style on every project on the GNOME platform. Anyway, if anyone wants to bug all the project devs and try to convince them to follow a certain style... :) Can we close this one?
We would like to move this documented to gnome-devel-docs, converting it to DocBook, and then update it. Nobody has volunteered to do that yet.
I'll port it to docbook.
Ooh. Thanks.
Created attachment 121577 [details] Programming guidelines, in Docbook. Here is is; with help from Claude Paroz.
Shaun, can this be added to gnome-devel-docs, please? I think we agree that that would be the best place for it.
Unassigning myself as there is a second part (needs updating) I won't handle.
*** Bug 481174 has been marked as a duplicate of this bug. ***
The programming guidelines have now been ported to Mallard: https://git.gnome.org/browse/gnome-devel-docs/log/programming-guidelines/C There has not been much further work on them, so they seem unmaintained at this point.
I’m spending a few days updating and adding things to them. Here’s a branch so far: http://cgit.collabora.com/git/user/pwith/gnome-devel-docs.git/log/?h=programming-guidelines-redux I’ll submit patches when I’m happy with things.
Created attachment 296288 [details] [review] platform-overview: Tweak licence page to mention LGPLv2.1+ over LGPLv2.1 LGPLv2.1= is a very odd licence to choose, and almost all projects go with LGPLv2.1+ instead. For consistency with the text below which mentions GPLv2+, add a ‘+’ to the LGPL sentence.
Created attachment 296289 [details] [review] programming-guidelines: Rework introduction to cover the GNOME ecosystem These guidelines are useful enough to cover development of any application which uses GNOME technologies — not just development of core GNOME applications. Rework the introduction to fit that a little better. Also cheekily add myself as an author in anticipation of further commits.
Created attachment 296290 [details] [review] programming-guidelines: Use ‘GNOME’ instead of ‘Gnome’ Capitalisation naziism.
Created attachment 296291 [details] [review] programming-guidelines: Add the licence to all pages Just to make things explicit.
Created attachment 296292 [details] [review] programming-guidelines: Move documentation section to its own page It isn’t really anything to do with C coding style, and can be expanded.
Created attachment 296293 [details] [review] programming-guidelines: Expand the documentation page Cover all the topics I can think of about modern usage of gtk-doc.
Created attachment 296294 [details] [review] programming-guidelines: Use styles to indicate valid code samples Rather than using comments at the top of the code example which could be missed or mistaken for part of the code sample. Also remove some extraneous whitespace and set the content type of the <code> elements.
Created attachment 296295 [details] [review] programming-guidelines: Add a synopsis to the documentation page
Created attachment 296296 [details] [review] programming-guidelines: Switch from guides to topics The pages containing the bulk of the text should be topics, not guides. Guides should be used for the overview pages instead.
Pushed all the cleanup patches after a rapid (highly appreciated) review on IRC by amigadave: > amigadave: the cleanup commits (up to the commit with the memory management page) seem completely fine to me > amigadave: likewise the platform-overview fix > amigadave: pwithnall: i say push those some bits, and then i can review the rest later I will clean up and attach the other patches next week for review. Attachment 296288 [details] pushed as 226af90 - platform-overview: Tweak licence page to mention LGPLv2.1+ over LGPLv2.1 Attachment 296289 [details] pushed as 73e4232 - programming-guidelines: Rework introduction to cover the GNOME ecosystem Attachment 296290 [details] pushed as f4f72a0 - programming-guidelines: Use ‘GNOME’ instead of ‘Gnome’ Attachment 296291 [details] pushed as e9ff575 - programming-guidelines: Add the licence to all pages Attachment 296292 [details] pushed as 891d151 - programming-guidelines: Move documentation section to its own page Attachment 296293 [details] pushed as 15e0fe7 - programming-guidelines: Expand the documentation page Attachment 296294 [details] pushed as 2c73514 - programming-guidelines: Use styles to indicate valid code samples Attachment 296295 [details] pushed as a93c458 - programming-guidelines: Add a synopsis to the documentation page Attachment 296296 [details] pushed as e198bb9 - programming-guidelines: Switch from guides to topics
Created attachment 296451 [details] [review] programming-guidelines: Fix title capitalisation on coding style page
Created attachment 296452 [details] [review] programming-guidelines: Use Unicode quotation marks and ellipses Because I’m a massive pedant.
Created attachment 296453 [details] [review] programming-guidelines: Split code examples to valid and invalid styles Add style attributes so that when we gain CSS support for differentiating ‘valid’ and ‘invalid’, everything will be in place already.
Created attachment 296454 [details] [review] programming-guidelines: Tidy up code examples on coding style page Add missing <code> elements and tidy up a few of the examples.
Created attachment 296455 [details] [review] programming-guidelines: Fix whitespace in code examples Trailing whitespace in <code> elements is not trimmed, and was making the coding style page look messy.
Created attachment 296456 [details] [review] programming-guidelines: Add example of aliasing switch cases To satisfysatisfysatisfysatisfysatisfysatisfysatisfysatisfysatisfy -Wswitch-enum.
Created attachment 296457 [details] [review] programming-guidelines: Add recommendation to use G_DECLARE_*_TYPE macros Use the new G_DECLARE_FINAL_TYPE and G_DECLARE_DERIVABLE_TYPE hotness macros from GLib.
Created attachment 296458 [details] [review] programming-guidelines: Remove recommendation to use GSlice Its performance advantage has, over the years, turned into a significant performance disadvantage against glibc’s malloc. It still has its gains on Windows, but not significant enough to recommend it. It is likely to be pulled from GLib in future.
Created attachment 296459 [details] [review] programming-guidelines: Remove outdated FIXMEs from coding style page
Created attachment 296460 [details] [review] programming-guidelines: Move documentation page FIXMEs to a comment
Created attachment 296461 [details] [review] programming-guidelines: Add a page on memory management
Created attachment 296462 [details] [review] programming-guidelines: Add a page on available tooling This is not meant as a tutorial on how to use any of the tools — merely as an overview of the available tools for each task, and which ones are currently recommended. It is expected that this page will evolve over time as tools come and go. Perhaps we should mention outdated tools for each task, and instruct the user to avoid them?
Created attachment 296463 [details] [review] programming-guidelines: Add a page on parallel installability This is heavily based on Havoc’s original article on parallel installability: http://ometer.com/parallel.html Confirmation has been obtained via e-mail that the original is licenced under CC-BY-SA 3.0.
Created attachment 296464 [details] [review] programming-guidelines: Add a page on namespacing APIs
Created attachment 296465 [details] [review] programming-guidelines: Add a page on databases and persistent data
Created attachment 296466 [details] [review] programming-guidelines: Add a page on accessing the file system
Created attachment 296467 [details] [review] programming-guidelines: Add a page on log output
Created attachment 296468 [details] [review] programming-guidelines: Add a page on threading and async operations
Created attachment 296469 [details] [review] programming-guidelines: Add a page on GObject introspection
Created attachment 296470 [details] [review] programming-guidelines: Add a page on source code version control
Created attachment 296471 [details] [review] programming-guidelines: Add a page on versioning APIs
Created attachment 296472 [details] [review] programming-guidelines: Add a page on GError
Created attachment 296473 [details] [review] programming-guidelines: Add a page on GList
Created attachment 296474 [details] [review] programming-guidelines: Add a page on pre- and post-conditions
Created attachment 296475 [details] [review] programming-guidelines: Add a page on unit testing
Created attachment 296476 [details] [review] programming-guidelines: Rearrange index page sections Split the pages up a little more in the index list, although the groupings are still fairly arbitrary.
Comment on attachment 296451 [details] [review] programming-guidelines: Fix title capitalisation on coding style page Looks good.
Comment on attachment 296452 [details] [review] programming-guidelines: Use Unicode quotation marks and ellipses This could probably be improved further by using a more British style of grammar, and avoiding the genetive (using a preposition instead). This change is good, however.
Comment on attachment 296453 [details] [review] programming-guidelines: Split code examples to valid and invalid styles A good nod in the direction of consistency.
Comment on attachment 296454 [details] [review] programming-guidelines: Tidy up code examples on coding style page No splinter, so: @@ -394,8 +395,8 @@ if (another_condition) do_bar ();</code> <p> - Even if C handles NULL equality like a boolean, be explicit. - This makes it easier to port your C code to something like + Even if C handles <code>NULL</code> equality like a boolean, be + explicit. This makes it easier to port your C code to something like C#, where testing against null explicitly is important: </p> Need a <code> for the "null" in the last sentence? Other than that, looks good.
Comment on attachment 296455 [details] [review] programming-guidelines: Fix whitespace in code examples Urgh, messy. But now tidy!
Comment on attachment 296456 [details] [review] programming-guidelines: Add example of aliasing switch cases Good suggestion. For situations where there are a huge number of cases (GstMessageType, for example), and the code only needs to handle a few, I have been switching to using "if() else" instead.
Comment on attachment 296457 [details] [review] programming-guidelines: Add recommendation to use G_DECLARE_*_TYPE macros + For derivable types, private data must be stored in a private struct in + the C file, configured using <code>G_DEFINE_TYPE_WITH_PRIVATE()</code> + and accessed using a <code>_get_instance_private()</code> function: I think that _get_instance_private() is a macro. I have marked this as commit_after_freeze because it needs to go to master, and a new gnome-3-14 branch should be created without the "new hotness".
Comment on attachment 296458 [details] [review] programming-guidelines: Remove recommendation to use GSlice Ryan would approve this patch, if he were around. :-)
Comment on attachment 296459 [details] [review] programming-guidelines: Remove outdated FIXMEs from coding style page Mmm, crufty.
Comment on attachment 296460 [details] [review] programming-guidelines: Move documentation page FIXMEs to a comment Yay for Mallard.
Comment on attachment 296461 [details] [review] programming-guidelines: Add a page on memory management + <p> + It is assumed that the reader is familiar with the idea of heap allocation + of memory using <code>malloc()</code> and <code>free()</code>, and knows of + the GLib equivalents, <code>g_malloc()</code> and <code>g_free()</code>. + </p> It is important to not the g_malloc() and g_free() must be paired, as with g_slice_new() and g_slice_free(). Although this is mentioned in the GLib API reference, and you do not encourage GSlice use here, this could bite API consumers. + Use reference counting rather than explicit finalisation where possible. + (<link xref="#reference-counting"/>) Probably "finalization", as we are in the C locale. + <p> + Ideally, all functions have a <code>(transfer)</code> annotation for all + relevant parameters and the return value. Failing that, here is a set of + guidelines to use to determine whether ownership of a return value is + transferred: + </p> Would be good to link to the list of known annotations here (on the wiki?). On the other hand, I just noticed that you did this later. Some reshuffling may make sense. + data type, e.g. <code>g_strdup()</code> for strings, or + <code>g_object_ref()</code> for GObjects. + </p> Not a big fan of Latin (e.g.), just plain English "for example" is better (and throughout the remainder of the text). + finalise the instance, it frees the current scope’s ownership of that + instance. "ize" (probably). + finalise the instance, it frees the current scope’s ownership of that + instance. Floating refs, maybe? Ugly to discuss here, but very important for GTK+. + All owned variables are set to <code>NULL</code> when initialised or "ized"? + function called in the code. Domain-specific static analysers like Tartan "analyzer"? Also, it would be worth mentioning the clang static analyzer. + Runtime leak checking is done using + <link href="http://valgrind.org/">Valgrind</link>, using its + <link href="http://valgrind.org/docs/manual/mc-manual.html"> Also, the new sanitizers in GCC (and clang) may be easier to use than Valgrind.
Comment on attachment 296462 [details] [review] programming-guidelines: Add a page on available tooling + other tools exist for more specialised use cases, and should be used when + appropriate. Usual "ise" versus "ize" comments for the C locale (and for the remainder of the text. Similarly, try to avoid Latin (e.g.). + They may have to remain in autoconf-archive, with that as a built time + dependency of the project: "built time" → "build time" + FIXME: Mention AX_IS_RELEASE here once it’s merged: This is done now, I think.
Attachment 296451 [details] pushed as c900545 - programming-guidelines: Fix title capitalisation on coding style page Attachment 296452 [details] pushed as 307c775 - programming-guidelines: Use Unicode quotation marks and ellipses Attachment 296453 [details] pushed as 1cc157d - programming-guidelines: Split code examples to valid and invalid styles Attachment 296454 [details] pushed as 1d3b1d8 - programming-guidelines: Tidy up code examples on coding style page Attachment 296455 [details] pushed as 1cf2316 - programming-guidelines: Fix whitespace in code examples Attachment 296458 [details] pushed as 2f50e53 - programming-guidelines: Remove recommendation to use GSlice Attachment 296459 [details] pushed as 030ae23 - programming-guidelines: Remove outdated FIXMEs from coding style page Attachment 296460 [details] pushed as baeb6f1 - programming-guidelines: Move documentation page FIXMEs to a comment
(In reply to David King from comment #50) > Comment on attachment 296454 [details] [review] [review] > programming-guidelines: Tidy up code examples on coding style page > > No splinter, so: > > @@ -394,8 +395,8 @@ if (another_condition) > do_bar ();</code> > > <p> > - Even if C handles NULL equality like a boolean, be explicit. > - This makes it easier to port your C code to something like > + Even if C handles <code>NULL</code> equality like a boolean, be > + explicit. This makes it easier to port your C code to something > like > C#, where testing against null explicitly is important: > </p> > > Need a <code> for the "null" in the last sentence? > > Other than that, looks good. I am using lowercase ‘null’ to refer to the concept, and uppercase ‘NULL’ to refer to the constant. The concept doesn’t need a <code>, so I’ve pushed the patch without changes.
Comment on attachment 296463 [details] [review] programming-guidelines: Add a page on parallel installability + at the same time is hard to organise and demoralising, as most API breaks "is" versus "iz". Otherwise, looks great!
Comment on attachment 296456 [details] [review] programming-guidelines: Add example of aliasing switch cases Pushed the aliasing-switch-cases patch with a minor change to mention using an if statement if the enumerated type is huge. Attachment 296456 [details] pushed as 3dcc0d6 - programming-guidelines: Add example of aliasing switch cases
(In reply to David King from comment #53) > Comment on attachment 296457 [details] [review] [review] > programming-guidelines: Add recommendation to use G_DECLARE_*_TYPE macros > > + For derivable types, private data must be stored in a private struct > in > + the C file, configured using <code>G_DEFINE_TYPE_WITH_PRIVATE()</code> > + and accessed using a <code>_get_instance_private()</code> function: > > I think that _get_instance_private() is a macro. Nope, it’s a static inline function, because they’re cool (and can actually be defined from macros). > I have marked this as commit_after_freeze because it needs to go to master, > and a new gnome-3-14 branch should be created without the "new hotness". Merged now we’ve branched. Attachment 296457 [details] pushed as 2993b16 - programming-guidelines: Add recommendation to use G_DECLARE_*_TYPE macros
Committed the tooling, memory management and parallel installability pages with suggested fixes applied. I’ve added a new section to tooling about the sanitisers, and linked to it from memory management. I’ve added a new section to memory management about floating refs. Attachment 296461 [details] pushed as 3d96e02 - programming-guidelines: Add a page on memory management Attachment 296462 [details] pushed as 74eb0f8 - programming-guidelines: Add a page on available tooling Attachment 296463 [details] pushed as 1575da0 - programming-guidelines: Add a page on parallel installability
Review of attachment 296464 [details] [review]: ::: programming-guidelines/C/namespacing.page @@ +76,3 @@ + </p></item> + <item><p> + Macros and #defines should use <code>UPPER_CASE_WITH_UNDERSCORES</code>. This applies to onstants in general, I suppose.
Review of attachment 296465 [details] [review]: Needs a corresponding Makefile change, but otherwise good.
Review of attachment 296466 [details] [review]: Needs a corresponding Makefile change, but otherwise good. ::: programming-guidelines/C/file-system.page @@ +36,3 @@ + <list> + <item><p> + Always use asynchronous I/O for file access. Following the mailing list discussion on this, weakening this statement to a strong recommendation seems like a good idea.
Review of attachment 296467 [details] [review]: Makefile change required, but otherwise fine.
Review of attachment 296468 [details] [review]: Everybody dance and sing "Makefile!". Otherwise, pretty good. ::: programming-guidelines/C/threading.page @@ +115,3 @@ + free_param (data->param2); + + g_slice_free (SomeBlockingFunctionData, data); Are we recommending GSlice now? ;-)
Review of attachment 296469 [details] [review]: "Makefile" ::: programming-guidelines/C/introspection.page @@ +24,3 @@ + + <p> + If two packages can be parallel installed, then they have no filenames in Is this copy-paste intentional? Doesn't quite seem to fit here. @@ +47,3 @@ + recommended to <link xref="documentation#introspection-annotations">add + introspection annotations to documentation comments</link> in program + code, as they clarify the documentation. Might be worth mentioning (somewhere) the approach of having an internal library in an application for the purposes of running gtk-doc over it to generate internal API documentation. @@ +126,3 @@ + languages don’t allow this kind of freedom. So in order for a C API to be + representable in a higher level language, it has to conform to the + behaviours supported by that language. "behaviors". :-(
Review of attachment 296470 [details] [review]: "Makefile".
Review of attachment 296471 [details] [review]: "Makefile". ::: programming-guidelines/C/versioning.page @@ +63,3 @@ + Libraries have two version numbers: a libtool version which tracks ABI + backwards compatibility, and a package version which tracks feature + changes. These are normally incremented in synchronisation, but should be We love "z" in C-ica. @@ +108,3 @@ + <code>AC_INIT()</code>, and the one which is typically known as the + project’s version number. For example, the Debian package for a library + will use the library’s package version. Package versions have the form This is mostly true. However, if different versions of a library are parallel-installable, the major version will often be included in the package name (as otherwise multiple versions of the package could not be installed simultaneously). This is package-manager specific, of course. @@ +250,3 @@ + <p> + The package archive generated by <cmd>make distcheck</cmd> can now be + uploaded to ftp.gnome.org or distributed in other ways. download.gnome.org is better.
Review of attachment 296472 [details] [review]: "Makefile". ::: programming-guidelines/C/gerror.page @@ +49,3 @@ + + <p> + Printing warnings to the console must not be done in library code: use a However, g_return_*_if_fail() does this (although criticals may be considered separately from warnings).
Review of attachment 296473 [details] [review]: "Makefile". Otherwise, good advice.
Review of attachment 296474 [details] [review]: "Makefile", otherwise good. Might be worth mentioning breaking on criticals or warnings as a debugging tactic (even if only as a TODO). ::: programming-guidelines/C/preconditions.page @@ +67,3 @@ + <p> + Adding pre- and post-condition assertions to code is as much about + ensuring the behaviour of each function is correctly and completely "behavior".
Review of attachment 296475 [details] [review]: "Makefile". ::: programming-guidelines/C/unit-testing.page @@ +80,3 @@ + Like <link xref="version-control">git commits</link>, each unit test + should be ‘as small as possible, but no smaller’, testing a single + specific API or behaviour. Each test case must be able to be run "behavior".
Review of attachment 296476 [details] [review]: Sure, seems like an improvement.
All comments applied. Makefile repeatedly fixed. Thanks for the reviews! I’ll open new bugs for any updates I make to fix up the FIXMEs. I think we can consider this one fixed now. :-) Attachment 296464 [details] pushed as 8b2beec - programming-guidelines: Add a page on namespacing APIs Attachment 296465 [details] pushed as 4497424 - programming-guidelines: Add a page on databases and persistent data Attachment 296466 [details] pushed as 1628fb9 - programming-guidelines: Add a page on accessing the file system Attachment 296467 [details] pushed as 100e4c3 - programming-guidelines: Add a page on log output Attachment 296468 [details] pushed as 7062acd - programming-guidelines: Add a page on threading and async operations Attachment 296469 [details] pushed as 22592f5 - programming-guidelines: Add a page on GObject introspection Attachment 296470 [details] pushed as d7f9d32 - programming-guidelines: Add a page on source code version control Attachment 296471 [details] pushed as 1114474 - programming-guidelines: Add a page on versioning APIs Attachment 296472 [details] pushed as 20e3391 - programming-guidelines: Add a page on GError Attachment 296473 [details] pushed as 3f96c40 - programming-guidelines: Add a page on GList Attachment 296474 [details] pushed as 7a8784c - programming-guidelines: Add a page on pre- and post-conditions Attachment 296475 [details] pushed as 9458c2a - programming-guidelines: Add a page on unit testing Attachment 296476 [details] pushed as c982a72 - programming-guidelines: Rearrange index page sections