Be more clear that g_return_if_fail is undefined behaviour

In particular, it is not incorrect to g_return_if_fail (..., FALSE)
in a function returning a "success" gboolean and a GError: "failure to
meet the preconditions is an error" takes precedence over the
GError documentation's guarantee that the error will be set on failure.

Bug: https://bugzilla.gnome.org/show_bug.cgi?id=660809
Reviewed-by: Emmanuele Bassi

2 files changed, 39 insertions, 8 deletions

diff --git a/glib/gerror.c b/glib/gerror.c
index 304320773..5953b5a82 100644
--- a/glib/gerror.c
+++ b/glib/gerror.c
@@ -309,9 +309,23 @@
  *   g_set_error() will complain if you pile up errors.
  *
  * - By convention, if you return a boolean value indicating success
- *   then %TRUE means success and %FALSE means failure. If %FALSE is
+ *   then %TRUE means success and %FALSE means failure.
+ *   <footnote><para>Avoid creating functions which have a boolean
+ *   return value and a GError parameter, but where the boolean does
+ *   something other than signal whether the GError is set.  Among other
+ *   problems, it requires C callers to allocate a temporary error.  Instead,
+ *   provide a "gboolean *" out parameter. There are functions in GLib
+ *   itself such as g_key_file_has_key() that are deprecated because of this.
+ *   </para></footnote>
+ *   If %FALSE is
  *   returned, the error must be set to a non-%NULL value.
- * 
+ *   <footnote><para>One exception to this is that in situations that are
+ *   already considered to be undefined behaviour (such as when a
+ *   g_return_val_if_fail() check fails), the error need not be set.
+ *   Instead of checking separately whether the error is set, callers
+ *   should ensure that they do not provoke undefined behaviour, then
+ *   assume that the error will be set on failure.</para></footnote>
+ *
  * - A %NULL return value is also frequently used to mean that an error
  *   occurred. You should make clear in your documentation whether %NULL
  *   is a valid return value in non-error cases; if %NULL is a valid value,
diff --git a/glib/gmessages.h b/glib/gmessages.h
index d0b8406a6..497c6690e 100644
--- a/glib/gmessages.h
+++ b/glib/gmessages.h
@@ -304,10 +304,18 @@ GPrintFunc      g_set_printerr_handler  (GPrintFunc      func);
  * g_return_if_fail:
  * @expr: the expression to check
  *
- * Verifies that the expression evaluates to %TRUE.  If the expression
- * evaluates to %FALSE, a critical message is logged and the current
- * function returns.  This can only be used in functions which do not
- * return a value.
+ * Verifies that the expression @expr, usually representing a precondition,
+ * evaluates to %TRUE. If the function returns a value, use
+ * g_return_val_if_fail() instead.
+ *
+ * If @expr evaluates to %FALSE, the current function should be considered to
+ * have undefined behaviour (a programmer error). The only correct solution
+ * to such an error is to change the module that is calling the current
+ * function, so that it avoids this incorrect call.
+ *
+ * To make this undefined behaviour visible, if @expr evaluates to %FALSE,
+ * the result is usually that a critical message is logged and the current
+ * function returns.
  *
  * If G_DISABLE_CHECKS is defined then the check is not performed.  You
  * should therefore not depend on any side effects of @expr.
@@ -320,8 +328,17 @@ GPrintFunc      g_set_printerr_handler  (GPrintFunc      func);
  * @val: the value to return from the current function
  *       if the expression is not true
  *
- * Verifies that the expression evaluates to %TRUE.  If the expression
- * evaluates to %FALSE, a critical message is logged and @val is
+ * Verifies that the expression @expr, usually representing a precondition,
+ * evaluates to %TRUE. If the function does not return a value, use
+ * g_return_if_fail() instead.
+ *
+ * If @expr evaluates to %FALSE, the current function should be considered to
+ * have undefined behaviour (a programmer error). The only correct solution
+ * to such an error is to change the module that is calling the current
+ * function, so that it avoids this incorrect call.
+ *
+ * To make this undefined behaviour visible, if @expr evaluates to %FALSE,
+ * the result is usually that a critical message is logged and @val is
  * returned from the current function.
  *
  * If G_DISABLE_CHECKS is defined then the check is not performed.  You