Bug 37741

+typedef enum {
+ MCP_DEBUG_NONE = 0,
+ MCP_DEBUG_ACCOUNT = 1 << 0,
+ MCP_DEBUG_ACCOUNT_STORAGE = 1 << 1,
+ MCP_DEBUG_DBUS_ACL = 1 << 2,
+ MCP_DEBUG_DISPATCH_OPERATION = 1 << 3,
+ MCP_DEBUG_DISPATCH_OPERATION_POLICY = 1 << 4,
+ MCP_DEBUG_LOADER = 1 << 5,
+ MCP_DEBUG_REQUEST = 1 << 6,
+ MCP_DEBUG_REQUEST_POLICY = 1 << 7,
+} McpDebugType;
+
+gboolean mcp_debugging (McpDebugType type);

I'd prefer mcp_is_debugging() and McpDebugFlags.

This takes an implementation detail (the set of debug domains) and makes it API - be careful with that sort of thing.

> #define DEBUG(format, ...) \
> - _mcp_debug ("%s: " format, G_STRFUNC, ##__VA_ARGS__)
> + if (mcp_debugging (MCP_DEBUG_TYPE)) \
> + g_debug ("%s: " format, G_STRLOC, ##__VA_ARGS__)

All function-like macros that expand to more than one statement should have their body wrapped in G_STMT_START { ... } G_STMT_END to avoid astonishing precedence. (If in doubt, copy from telepathy-glib...)

> +++ b/mission-control-plugins/debug.h
> +#ifdef ENABLE_DEBUG

Not in a public header, please. Public headers should never rely on anything from config.h, because anyone could #include them (not just MC itself).

In-tree plugins should get their MCP_DEBUG() (or just DEBUG() - up to you, because it shouldn't be public API anyway, for this reason) from an in-tree-only header like "mission-control-plugins/debug-internal.h".

Out-of-tree plugins should have their own debug-internal.h (per project) that does its own #ifdef, which depends whether the project providing the plugin has been compiled with debug enabled.

Rule of thumb: none of the observable functionality of a library should depend on its configure options, and its API certainly shouldn't "change shape". (It'd be OK if mcp_is_debugging() always returned FALSE if compiled without debugging, or if mcp_debug() did nothing if compiled without debugging, or something.)

> Squash a recent compiler warning
> + {

This commit shouldn't have been needed. Use G_STMT_START properly and it can be removed.

> Call g_key_file_remove... rather than ...set_value when new value is NULL
> Don't call tp_intset_size on unset intset

r+ for these, please cherry-pick them.

(In reply to comment #1)

> This takes an implementation detail (the set of debug domains) and makes it API
> - be careful with that sort of thing.

Updated. I figured that all I was exposing here was one flag value for
each plugin interface (and of course the plugin interfaces have to be
exposed to the user or they can't implement them...)

I think I got all the other r- comments.

r- removed since it's up for re-review.

> plugin debug header should be included via the main plugin header

If this is your intention, please make including it directly just fail.

> +#include <mission-control-plugins/mission-control-plugins.h>
> +#include <mission-control-plugins/debug.h>
> +#include <config.h>

config.h should always come first, and debug.h is now redundant.

Neither of those is critical.

Amended as per final comment and merged.

2375 % git tag --contains=0cfe51094842
telepathy-mission-control-5.8.0
telepathy-mission-control-5.9.1