Bug 33660

+  g_return_val_if_fail (self != NULL, FALSE);
+  g_return_val_if_fail (TP_IS_DEBUG_SENDER (self), FALSE);

Just ooi, why both of these?

Go go though.

(In reply to comment #1)
> +  g_return_val_if_fail (self != NULL, FALSE);
> +  g_return_val_if_fail (TP_IS_DEBUG_SENDER (self), FALSE);
> 
> Just ooi, why both of these?
> 
> Go go though.

I believed that TP_IS_DEBUG_SENDER (NULL) returned TRUE. But I am wrong.

It has occurred to me that a different way to do this would be to have a variation on the theme of tp_debug_sender_add_message() like this:

tp_debug_sender_add_message_vprintf (TpDebugSender *self,
    GLogLevelFlags level,
    const gchar *domain,
    const gchar *format,
    va_args args)
{
  GTimeVal now;
  gchar *message;

#if DISABLE_SPECULATIVE_DEBUG_LOGGING
  if (!self->priv->enabled)
    return;
#endif

  va_start (args, format);
  message = g_strdup_vprintf (format, args);
  va_end (args);
  g_get_current_time (&now);

  tp_debug_sender_add_message (dbg, &now, domain, level, message);

  g_free(message);
}

This would mean formatting the message twice in the worst case (once for calling g_log() directly, once in tp_debug_sender_add_message_vprintf) but would mean that speculative logging could be turned off in one place for the entire platform.

If we were feeling particularly advanced, this function could return the message if it does build it. So the caller could say:

  gchar *message = tp_debug_sender_add_message_vprintf()

  if (flag & flags || level > G_LOG_LEVEL_DEBUG)
    {
      if (message == NULL)
        {
          va_start (args, format);
          message = g_strdup_vprintf (format, args);
          va_end (args);
        }
      g_log(...)
    }

Now we're getting into micro-optimizations though.

Attempt #2 after discussion with wjt.

+ * tp_debug_sender_vprintf_message:
[snip]
+ * tp_debug_sender_printf_message:

I preferred Will's function name suggestions, add_message_vprintf and add_message_printf.

+#ifndef ENABLE_DEBUG_CACHE
+  if (self->priv->enabled || formatted != NULL)
+    {
+#endif

Instead of indenting the whole function couldn't you do:

#ifndef ENABLE_DEBUG_CACHE
  if (!self->priv->enabled && formatted == NULL)
    return;
#endif

+      if ((when = timestamp) == NULL)

Can we just split this into two lines please?

+ * formats and adds a new message to the debug sender message queue. If the

Capital F!

+ * @timestamp: Time of the message, or %NULL for right now

Just out of interest, why is this different from _add_message in that it doesn't have a fallback to now? It'd be cool if they were all consistent, whatever the behaviour is.

+ * Since: UNRELEASED

Usual style is major.minor.UNRELEASED but it doesn't really matter as it's going to get sedded out later anyway.

You haven't added the new functions to the gtk-doc sections file so I'm guessing you haven't run make check or distcheck?

> function name suggestions, add_message_vprintf and add_message_printf.
Will rename then, I guess.

> +#ifndef ENABLE_DEBUG_CACHE
> +  if (self->priv->enabled || formatted != NULL)
> +    {
> +#endif
> 
> Instead of indenting the whole function couldn't you do:

Done. the code there used to be a little different, the 
enclosing {} made sense before, but it's superfluous now.

> +      if ((when = timestamp) == NULL)
> Can we just split this into two lines please?

done.

> + * formats and adds a new message to the debug sender message queue. If the
> Capital F!

Done.

> + * @timestamp: Time of the message, or %NULL for right now
> 
> Just out of interest, why is this different from _add_message in that it

add_message didn't have a fallback before, for no really good reason except
that was how it was written. They all fall back now.


> + * Since: UNRELEASED
> Usual style is major.minor.UNRELEASED but it doesn't really matter as it's
> going to get sedded out later anyway.

> You haven't added the new functions to the gtk-doc sections file so I'm
> guessing you haven't run make check or distcheck?

check passes.
distcheck fails in a completely unrelated file that I haven't even gone near.

Ready for re-review

Looks OK.

Vivek merged this.