GNOME Bugzilla – Bug 744305
improve the debugging setup
Last modified: 2015-02-18 10:47:32 UTC
It would be great if we could use switches to turn on/off the debugging of various components in the application. Currently the application uses g_debug which gets us everything in one big mess, that's confusing sometimes when we don't want to see *all* the debug output and are only focusing on one component.
Created attachment 296629 [details] [review] debug: Add basic debug infrastructure
Created attachment 296630 [details] [review] debug: Change whole source code to use new debug setup Replaces all g_debug with new _photos_debug_print with appropriate flags.
Above patches make use of enable debug variable (ENABLE_DEBUG) introduced by Bug 744309 ,
Review of attachment 296629 [details] [review]: Thanks for the patch, Pranav. A few comments below. ::: src/photos-debug.c @@ +29,3 @@ +#include "photos-debug.h" + +PhotosDebugFlags _photos_debug_flags; Should be marked as static since there is no need to expose it as extern. See below ... @@ +44,3 @@ + keys, G_N_ELEMENTS (keys)); +#endif /* ENABLE_DEBUG */ +} Add a photos_debug which is similarly wrapped with ENABLE_DEBUG. ::: src/photos-debug.h @@ +24,3 @@ + +#ifndef ENABLE_DEBUG_H +#define ENABLE_DEBUG_H Should be PHOTOS_DEBUG_H @@ +36,3 @@ +} PhotosDebugFlags; + +void _photos_debug_init(void); No need for the leading underscore and missing space before the opening parenthesis. @@ +39,3 @@ + +extern PhotosDebugFlags _photos_debug_flags; +static inline gboolean _photos_debug_on (PhotosDebugFlags flags) G_GNUC_CONST G_GNUC_UNUSED; We don't need to expose these in the header. They can be hidden inside photos-debug.c. See below ... @@ +45,3 @@ +{ + return (_photos_debug_flags & flags) == flags; +} I am curious why you preferred the gnome-terminal pattern. The way empathy does it looks more sane to me. For example, we don't need to put most of this in the header, nor do we need the macro magic towards the end. Even empathy has too many bells and whistles that we don't need, but the general pattern is cleaner. We only need two public functions: 1) photos_debug_init (or photos_debug_set_flags): To parse the environment variable using g_parse_debug_string. In fact, I like the idea of passing the name of the environment variable instead of hard coding it. Makes it easier to create a sister application out of the same source tree - the way gnome-books lives with gnome-documents. 2) photos_debug: Which checks the flag and uses g_debug to output the message. The guts of both functions would be wrapped in ENABLE_DEBUG and the flags variable does not need to be exposed in the header. @@ +51,3 @@ +#else +#define _PHOTOS_DEBUG_IF(flags) if (0) +#endif We don't need _PHOTOS_DEBUG_IF at this point. Let's not add too many things that we don't need. @@ +55,3 @@ +#if defined(__GNUC__) && G_HAVE_GNUC_VARARGS +#define _photos_debug_print(flags, fmt, ...) \ + G_STMT_START { _PHOTOS_DEBUG_IF(flags) g_printerr(fmt, ##__VA_ARGS__); } G_STMT_END We can just use g_debug here, and there is no need for this macro magic as mentioned above. A macro does let us do clever things like printing the filename and line number, but this code is not doing that, and we don't really need it either. The less pre-processor magic the better. @@ +72,3 @@ +G_END_DECLS + +#endif /* !ENABLE_DEBUG_H */ /* PHOTOS_DEBUG_H */
Created attachment 296732 [details] [review] build: Add ENABLE_DEBUG macro to use for debugging
Created attachment 296733 [details] [review] debug: Add basic debug infrastructure
Created attachment 296734 [details] [review] debug: Change whole source code to use new debug setup Replaces all g_debug with new _photos_debug_print with appropriate flags.
Review of attachment 296732 [details] [review]: ::: configure.ac @@ +18,3 @@ AX_IS_RELEASE([git-directory]) +AX_CHECK_ENABLE_DEBUG(,[ENABLE_DEBUG],,[$ax_is_release]) Two things: a) The Wiki [1] used to be wrong in saying that the default is "yes". It is actually "no". Philip confirmed this and you can read the M4 sources too. The Wiki has since been updated. b) Philip and I [2] decided against wrapping our debug code with #ifdef ENABLE_DEBUG, even though other modules might be doing that. Being able to turn the debug messages at run time is very valuable (even on release builds), which won't be possible if we compile them out. Also, none of the debug messages occur in performance sensitive code, so compiling them out does not yield any practical benefits. [1] https://wiki.gnome.org/Projects/GnomeCommon/Migration [2] see the email thread
Review of attachment 296733 [details] [review]: Much better. ::: src/photos-debug.c @@ +22,3 @@ + +#include <glib.h> +#include <stdarg.h> Include stdarg.h before glib.h and put a newline between them. We generally separate out config.h, standard C headers, library headers and internal headers. @@ +23,3 @@ +#include <glib.h> +#include <stdarg.h> +#include <glib/gstdio.h> We don't need to include glib/gstdio.h any more. @@ +32,3 @@ +void photos_debug (guint flags, const char *fmt, ...) +{ +#ifdef ENABLE_DEBUG We decided to just drop the #ifdefs. See the other review. @@ +34,3 @@ +#ifdef ENABLE_DEBUG + gchar *message; + va_list ap; Extra space before 'ap'. ::: src/photos-debug.h @@ +26,3 @@ +G_BEGIN_DECLS + +typedef enum { The opening brace should in a separate line. See src/photos-query.h @@ +33,3 @@ + +void photos_debug_init (void); +void photos_debug (guint flags, const char *fmt, ...); Missing newline between function prototypes, and please align the return types and parameters.
Review of attachment 296734 [details] [review]: ::: src/gegl-gtk-view-helper.c @@ +18,3 @@ */ +#include "config.h" Do we really need to include config.h for this change? You are right in that every .c file should ideally include config.h, but if this is not mandatory for this change, then please do it in a separate patch. @@ +24,3 @@ +#include "photos-debug.h" +#include "gegl-gtk-view-helper.h" Ditto. See above. ::: src/photos-dlna-renderer.c @@ +66,3 @@ if (error != NULL) \ { \ + photos_debug (PHOTOS_DEBUG_DLNA, "%s: %s: %s", G_STRFUNC, msg, error->message); \ The original code was wrong. It should have used g_warning instead of g_debug because it is reporting a failure. Can you please change it to g_warning from g_debug in a separate patch?
Created attachment 296863 [details] [review] build: Add -g -O0 by specifying yes Lets not use enable debug variables at all because : 1. We are not using them in the code as of now. 2. Even if we have to use them in future, we will use more-standard NDEBUG variable which is the default when $3 is empty.
Created attachment 296864 [details] [review] debug: Add basic debug infrastructure
Created attachment 296865 [details] [review] dlna-renderer: Use g_warning for reporting failure
Created attachment 296866 [details] [review] debug: Use the new debug setup
Review of attachment 296865 [details] [review]: Please add the bugzilla URL before pushing.
Created attachment 296872 [details] [review] dlna-renderer: Use g_warning for reporting failure
Review of attachment 296863 [details] [review]: The code change is OK, but the commit message is not. The important thing to mention is that we are turning debug on by default. You could explain that the default level in AX_CHECK_ENABLE_DEBUG is actually "no" and the Wiki did not correctly mention it, which is why this change was required. The current commit message literally describes the code change and states the obvious, but says nothing about why we suddenly wanted to add the '[yes]'.
Review of attachment 296872 [details] [review]: Perfect.
Created attachment 296893 [details] [review] build: Turn debug on by default
Review of attachment 296893 [details] [review]: Perfect. Thanks, Pranav.
Review of attachment 296864 [details] [review]: ::: src/photos-debug.c @@ +40,3 @@ + + if (photos_debug_flags & flags) + g_debug (message); This will trigger -Wformat-security. For printf style functions it is generally better to use this form: func ("%s", str) ... instead of: func (str) Sorry for not pointing this out earlier. @@ +55,3 @@ + + photos_debug_flags = g_parse_debug_string (g_getenv ("GNOME_PHOTOS_DEBUG"), + keys, G_N_ELEMENTS (keys)); Nitpick. This could be in the same line. ::: src/photos-debug.h @@ +30,3 @@ + PHOTOS_DEBUG_GEGL = 1 << 0, + PHOTOS_DEBUG_TRACKER = 1 << 1, + PHOTOS_DEBUG_DLNA = 1 << 2, We need something for the debug messages in the PhotosBaseItem implementations. @@ +35,3 @@ +void photos_debug_init (void); + +void photos_debug (guint flags, const char *fmt, ...); The parameter lists (ie. the opening parentheses) should also be aligned. Another thing to keep in mind when aligning like this is to leave some room for future expansion. ie. have more than a single whitespace between the widest columns. Otherwise we will have to realign them all when we add a new row with a wider entry.
Created attachment 296894 [details] [review] debug: Add basic debug infrastructure I fixed everything except the flag for the PhotosBaseItem implementations. Pranav, do you have any suggestions as to what it could be? I can think of "network".
Review of attachment 296866 [details] [review]: It looks fine, except that we have left out the g_debugs in the PhotosBaseItem implementations. ::: src/gegl-gtk-view-helper.c @@ +24,3 @@ +#include "photos-debug.h" +#include "gegl-gtk-view-helper.h" + Nitpick. Better to move the headers around in a separate patch.
Created attachment 296895 [details] [review] debug: Use the new debug setup Removed the chunk about the #includes, but we need to cover the remaining g_debugs.
Created attachment 296909 [details] [review] debug: Add basic debug infrastructure Silence -Wsuggest-attribute=format by using G_GNUC_PRINTF.
Comment on attachment 296893 [details] [review] build: Turn debug on by default Attachment 296893 [details] pushed as 440180f - build: Turn debug on by default
Created attachment 296931 [details] [review] debug: Add basic debug infrastructure
Created attachment 296932 [details] [review] debug: Use the new debug setup Replaces all g_debug with photos_debug along with appropriate flags.
Created attachment 296934 [details] [review] gegl-gtk-view-helper: Change the ordering of #includes Let's follow the convention and separate out config.h, standard C headers, library headers and internal headers.
Created attachment 296935 [details] [review] gegl-gtk-view-helper: Change the ordering of #includes Let's follow the convention and separate out config.h, standard C headers, library headers and internal headers. Additionaly, add missing config.h to list of includes.
Review of attachment 296935 [details] [review]: Perfect.
Review of attachment 296931 [details] [review]: Looks good, except ... ::: src/photos-debug.c @@ +40,3 @@ + { "tracker", PHOTOS_DEBUG_TRACKER }, + { "dlna", PHOTOS_DEBUG_DLNA }, + { "network", PHOTOS_DEBUG_NETWORK }, You either align them, or you don't. Aligning them makes them look nice, but if you add a new one then you might have to realign. It is a trade-off, and you pick one or the other. I prefer to align the stuff in headers, but not the private ones in the C files.
Review of attachment 296932 [details] [review]: You can drop the debug prefix from the subject because you haven't touched photos-debug.[ch]. ::: src/photos-tracker-controller.c @@ +225,3 @@ else { + photos_debug (PHOTOS_DEBUG_TRACKER, "Query Elapsed: %" G_GINT64_FORMAT, (now - priv->last_query_time) / 1000000); You might want to wrap this line. It is getting too close to the 120 char limit and wraps around when seeing the diff in git.
Created attachment 297083 [details] [review] Add basic debug infrastructure
Created attachment 297084 [details] [review] Use the new debug setup