Created attachment 291997 [details] [review]
macros: add side-effecting variants of asserts

Review of attachment 291997 [details] [review]:

Cool!

TBH, they do come at extra cost in terms of cognitive load.

are we supposed to be replacing all g_return_* and g_assert() calls?

while I do see some of the point of having an unconditional g_return_* pair, what would be the purpose of a g_assert_se()?

shouldn't we just ignore G_DISABLE_ASSERT if we don't want to have conditional assertions?

I'm with ebassi on this one.

Another variant of macros doing almost, but not quite the same ? Just say no...

Seems confusing to me, and it's easy enough to just keep the side-effecting code separate from the assertion...

And our asserting macro situation is already too complicated:

  - g_return_val_if_fail(expr,val) - only checks @expr, logs a warning,
      and returns if !G_DISABLE_CHECKS, otherwise it's a no-op

  - g_return_val_if_reached(val) - only logs a warning if !G_DISABLE_CHECKS,
      but returns @val either way

  - g_warn_if_fail(expr) - always checks @expr and logs a warning, regardless
      of G_DISABLE_CHECKS

And this would add:

  - g_return_val_if_fail_se(expr,val) - always checks @expr, but only logs
      a warning and returns if !G_DISABLE_CHECKS


(FTR, the g_return_if_fail_se() docs refer to @val, which doesn't exist.)

Consider this code that I just wrote in the gvariant-kdbus branch:

/**
 * g_bytes_new_take_zero_copy_fd:
 * @fd: a file descriptor capable of being zero-copy-safe
 *
 * Creates a new #GBytes from @fd.
 *
 * @fd must be capable of being made zero-copy-safe.  In concrete terms,
 * this means that a call to g_unix_fd_ensure_zero_copy_safe() on @fd
 * will succeed.  This call will be made before returning.
 *
 * This call consumes @fd, transferring ownership to the returned
 * #GBytes.
 *
 * Returns: (transfer full): a new #GBytes
 *
 * Since: 2.44
 */
GBytes *
g_bytes_new_take_zero_copy_fd (gint fd)
{
  GBytesData *bytes;
  struct stat buf;

  g_return_val_if_fail_se (g_unix_fd_ensure_zero_copy_safe (fd), NULL);

  ...


g_unix_fd_ensure_zero_copy_safe() is an expensive function (it does syscalls) that checks if an fd is sealed and, if not, seals it.

I want to call this expensive function as the first step of g_bytes_new_take_zero_copy_fd().  I want the side effect (ie: the seal-it-if-not-already feature).  I want to return with a critical failure if that fails.

Furthermore, I want the critical failure message to show that the failure is because that function failed.  I don't want to write this:

  gboolean successful;

  successful = g_unix_fd_ensure_zero_copy_safe (fd);
  if (!successful)
    g_critical (<<try to remember the correct format of the message>>);

Additionally, our testsuite has no shortage of code that assigns to a boolean variable like 'res' or 'ok' and then does g_assert(ok); in order to avoid the side effects being removed...

(In reply to comment #5)
> Seems confusing to me, and it's easy enough to just keep the side-effecting
> code separate from the assertion...
> 
> And our asserting macro situation is already too complicated:
> 
>   - g_return_val_if_fail(expr,val) - only checks @expr, logs a warning,
>       and returns if !G_DISABLE_CHECKS, otherwise it's a no-op

This is reasonable.  I look at it from the standpoint of a programmer as "this may log a message and return, or maybe not".  That's fine because this is only supposed to happen in cases of undefined behaviour anyway.

> 
>   - g_return_val_if_reached(val) - only logs a warning if !G_DISABLE_CHECKS,
>       but returns @val either way

This also seems consistent.  I'd argue that the returning of the value being unconditional is a bit weird, but that was probably done because

 (a) there is no check here so 'just return' is always very cheap

 (b) avoid warnings about no return at end of function if you put it there

Either way, it fits nicely with my "all bets off" idea above.  Again, we should only be where we are in cases of programmer error.

>   - g_warn_if_fail(expr) - always checks @expr and logs a warning, regardless
>       of G_DISABLE_CHECKS

I consider this to be in a whole different category.  This logs a warning (G_LOG_LEVEL_WARNING), which is not something that's done in case of programmer error, but rather in response to unexpected external conditions.  We wouldn't want this to be disabled if G_DISABLE_CHECKS was defined.  Similar argument for g_warn_if_reached() btw.

I do notice that these functions are documented as "logs a critical warning".  That much is certainly confusing and we should fix the documentation.

fwiw, the only inconsistency that I see in this list of examples is the unconditional return on the g_return_{,val_}if_reached() macros, and this inconsistency is within the expected "undefinedness" of the function (modulo G_DISABLED_CHECKS) anyway.

I also want to point out that g_return_if_reached_se() makes no sense...

> (FTR, the g_return_if_fail_se() docs refer to @val, which doesn't exist.)

thanks

Created attachment 292030 [details] [review]
g_warn_if_*: clarify documentation

Clarify that these functions emit G_LOG_LEVEL_WARNING and not _CRITICAL.

Document that they are not affected by G_DISABLE_CHECKS.

See also bug 741049.

(In reply to comment #7)
> Additionally, our testsuite has no shortage of code that assigns to a boolean
> variable like 'res' or 'ok' and then does g_assert(ok); in order to avoid the
> side effects being removed...

It makes no sense to compile the test programs with assertions disabled, so no one should be worrying about side-effects being removed from them. I think g_assert(ok) is more of a stylistic thing; separating out the parts of the code that are actually doing stuff from the parts that are just verifying that the right stuff got done. (Which I guess is also why I don't like the _se() macros; it's confusing to have a precondition check that actually changes things in addition to checking a precondition.)

More examples that I believe to be useful:

            /* This is a memfd, so we know we won't have any trouble... */
            g_assert_se (munmap (data_bytes->data, bytes->size) == 0);
            g_assert_se (close (bytes->type_or_fd) == 0);

Comment on attachment 291997 [details] [review]
macros: add side-effecting variants of asserts

Do not mark the patch as accepted-commit_now, since several maintainers don't like the idea.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/965.