GNOME Bugzilla – Bug 735887
Follow GLib/GTK+ best practices for the order of #include's
Last modified: 2014-10-15 17:56:02 UTC
See https://wiki.gnome.org/Projects/GTK%2B/BestPractices/GlibIncludes The most important thing is to first include the corresponding header in a .c file.
One exception though for including the config.h, see bug #640449.
If you are asking for confirmation, sure, I am ok with refactoring and cleaning up the headers if you feel like doing it.
No, I filed the bug for adding one more gnome-love bug.
Created attachment 288269 [details] [review] Follow GLib/GTK+ best practices for the order of includes https://bugzilla.gnome.org/show_bug.cgi?id=735887 Refactored includes to follow the glib best practices: * config.h (if required) * own module header file * system header files * required headers from current project
Review of attachment 288269 [details] [review]: hi Robert, somme missing ones : gedit-marshal.c:3 Tab to replace by space gedit-marshal.c:4 Not needed blank line gedit-menu-extension.c:20 Not needed blank line gedit-menu-extension.c Need blank line between 22 and 23 gedit-multi-notebook.c Need blank line between 23 and 24 gedit-view-activatable.c Need blank line between 25 and 26 Other things we can probably do : - put each part in alphabetical order - as in some gedit .c, clearly separate gnome platform #include from others I haven't look at possible work in sub-dirs ( libgd, plugins ... ) ::: gedit/gedit-app-osx.c @@ -24,1 +25,1 @@ #include <gedit/gedit-dirs.h> maybe : #include "gedit-dirs.h" #include "gedit-debug.h" and put them in the right place. ::: gedit/gedit-view.c @@ -32,1 +34,1 @@ blank line not needed ::: gedit/gedit-window-activatable.c @@ -27,2 +27,4 @@ #include <string.h> +#include "gedit-window.h" + blank line not needed ::: gedit/gedit.c @@ -41,0 +36,6 @@ +#include <glib.h> +#include <locale.h> +#include <libintl.h> ... 3 more ... blank line not needed
(In reply to comment #5) > Other things we can probably do : > > - put each part in alphabetical order Not really worth the effort. The most important part is to first include the corresponding header (after config.h if needed). > - as in some gedit .c, clearly separate gnome platform #include from others That should already be the case if following GLib/GTK+ conventions.
Created attachment 288270 [details] [review] Follow GLib/GTK+ best practices for the order of includes https://bugzilla.gnome.org/show_bug.cgi?id=735887 Refactored includes to follow the glib best practices: * config.h (if required) * own module header file * system header files * required headers from current project
It would be great if we had a tool that could check (or even automate) this, so that we keep the includes conformant. Otherwise I'm afraight it will deteriorate as development continues.
(In reply to comment #8) > It would be great if we had a tool that could check (or even automate) this, so > that we keep the includes conformant. Otherwise I'm afraight it will > deteriorate as development continues. GLib doesn't have such a tool either, as far as I can tell. I'm not sure it would be easy to implement, but I guess anyone could use such a tool, so it could be shared for all GNOME modules. But until that happens, what do you think about adding the guidelines to the HACKING file?
In the HACKING file there is already a link to: https://wiki.gnome.org/Projects/GTK%2B/BestPractices which has a link to the right page: https://wiki.gnome.org/Projects/GTK%2B/BestPractices/GlibIncludes The links can anyway quickly become outdated, so only one link is enough.
robert : still miss the gedit-marshal.c changes Jessvdk : for gedit-app-osx.c, can we change : #include <gedit/gedit-dirs.h> #include <gedit/gedit-debug.h> with #include "gedit-dirs.h" #include "gedit-debug.h" and of course put it in the right place ? I don't know if it can hurt your build system for osx.
(In reply to comment #11) > robert : still miss the gedit-marshal.c changes gedit-marshal.c is built source, it's not part of the repository, I'm not sure how to do that. Or am I missing something?
robert : ho, sorry, my bad. It's generated by glib-genmarshal from glib tools, so forget about that. We way for Jessevdk opinion on gedit-app-osx.c and it's fine.
(In reply to comment #11) > for gedit-app-osx.c, can we change : > > #include <gedit/gedit-dirs.h> > #include <gedit/gedit-debug.h> > > with > > #include "gedit-dirs.h" > #include "gedit-debug.h" > > I don't know if it can hurt your build system for osx. There is no particular reason to use the former. Those headers are in the same directory, so the latter will work fine. There is nothing specific for OS X here, it's just C code.
robert : can you update your patch with this ^^ ? ( using: #include "gedit-dirs.h" #include "gedit-debug.h" )
Created attachment 288564 [details] [review] Follow GLib/GTK+ best practices for the order of includes Refactored includes to follow the glib best practices: * config.h (if required) * own module header file * system header files * required headers from current project
Update previous patch with two changes: * include <gedit/gedit-(dirs|debug).h> changed to "gedit-(dirs|debug).h in gedit/gedit-app-osx.c * commit message changed to include the bug url at the end of the commit message instead of the second line
Review of attachment 288564 [details] [review]: fine, push it
Comment on attachment 288564 [details] [review] Follow GLib/GTK+ best practices for the order of includes Pushed to master.
commited as 039d243