Returning TRUE for zero makes no sense. Obviously, zero is not a power
of two.
Also, the function is used to check whether a number has only one bit
(flag) set, so, an alternative name would be "has-one-bit-set", which
also should return FALSE for zero. All callers didn't really care for
the previous meaning "has-at-most-one-bit-set".
This also avoids the issue of checking (x >= 0), which causes
-Wtype-limits warnings for unsigned types. Which was avoided
by doing (x == 0 || x > 0), which caused -Wlogical-op warning,
which then was avoided (x == 0 || (x > 0 && 1)). Just don't.
We recently added -Wlogical-op in our build process
(commit #41e7fca59762dc928c9d67b555b1409c3477b2b0).
Seems that old versions of gcc (4.8.x) will hit that warning with our
implementation of our "nm_utils_is_power_of_two" and
"test_nm_utils_is_power_of_two_do" macros.
Fool it just adding an always TRUE check.
I used to use g_strv_length ((char **) p) instead, but that feels
ugly because it g_strv_length() is not designed to operate on
arbitrary pointer arrays.
_NM_GET_PRIVATE() macro is used to implement a standard private-getter, but it
requires that "self" is a pointer of either "const type *" or "type *". That
is great in most cases, but sometimes we have predominatly self pointers of
different type, so it would require a lot of casts.
Add a different form _NM_GET_PRIVATE_VOID() where self pointer can be any
non-const pointer and returns a non-const private pointer after casting.
NM_CACHED_QUARK_FCN() is a replacement for G_DEFINE_QUARK().
G_DEFINE_QUARK() is mostly used to define GError quarks. As
such, it always appends _quark() to the function name, which
is unfavorable because it makes it harder to grep for the
definition of the function.
In general I think that macros that defined symbols by concatenating
something should be avoided because that makes it harder to locate
where the symbol was defined.
Similar to systemd's PROTECT_ERRNO. The difference it, that it doesn't
treat the auto-variable as internal, so it is allowed to use it. E.g.
if (!(fd = open (...)) {
NM_AUTO_PROTECT_ERRNO (errno_saved);
printf ("error: %s", g_strerror (errno_saved));
return FALSE;
}
We already have gs_fd_close, which however doesn't preserve
errno and only checks for fd != -1. Add our own define.
Downside is, we have to include stdio.h and errno.h,
which effectively ends up to be included *everywhere*.
We usually don't build NM with g_assert() disabled (G_DISABLE_ASSERT).
But even if we would, there is no assertion macro that always evaluates
the condition for possible side effects.
I think that is a useful thing to have.
It was a macro to pass on the non-const-ness of the argument, but
that just doesn't make sense.
That is a signature
char *nm_str_not_empty (char *)
does not make sense, because you cannot transfer ownership
conditionally without additional checks to avoid a leak. Which makes
this form is pointless. For example:
char *
foo (void)
{
char *s;
s = _create_value ();
return nm_str_not_empty (s); /* leaks "" */
}
g_assert() uses G_LIKELY(), which in turn uses _G_BOOLEAN_EXPR().
As glib's version of _G_BOOLEAN_EXPR() uses a local variable
_g_boolean_var_, we cannot nest a G_LIKELY() inside a G_LIKELY(),
or inside a g_assert(), or a g_assert() inside a g_assert().
Workaround that, by redefining the macro.
I already encountered this problem before, when having a nm_assert()
inside a ({...}) block, inside a g_assert(). Then I just avoided that
combination, but this situation is quite easy to encounter.
The "shared" directory contains files that are possibly used by all components
of NetworkManager repository.
Some of these files are even copied as-is to other projects (VPN plugins, nm-applet)
and used there without modification. Move those files to a separate directory.
By moving them to a common directory, it is clearer that they belong
together. Also, you can easier compare the copied versions to their
original via
$ diff -r ./shared/nm-utils/ /path/to/nm-vpn-plugin/shared/nm-utils/
