Passing in the func, file and line information complicates the
interface. On top of that, it prevents forward declaring Hashmap in
strv.h, as we need to pass the macros everywhere that we allocate a
hashmap, which means we have to include the hashmap header everywhere
we have a function that allocates a hashmap instead of just having to
forward declare Hashmap.
Let's drop the file, func and line information from the debug information.
Instead, in the future we can add a description field to hashmaps like we
already have in various other structs to describe the purpose of the hashmap
which should be much more useful than having the file, line and function where
the hashmap was allocated.
I have been trying to run
https://github.com/include-what-you-use/include-what-you-use on the
systemd repository to hopefully get a handle on the rampant numbers of
includes we have in every file with no idea if any of the symbols coming
from that file are used or not.
While I haven't got it fully working yet, these changes still make sense
IMO and can be merged already.
Except the last commit, all other changes are about removing circular
dependencies between headers which trips up include-what-you-use.
Regardless of the tool, circular dependencies between headers are a code
smell and I think we should get rid of them regardless of whether we end
up using the tool or not.
Now that the necessary functions from log.h have been moved to macro.h,
we can stop including log.h in macro.h. This requires modifying source
files all over the tree to include log.h instead.
Currently, hashmap_dump_sorted sorts by key and then returns the values
in order sorted by key. This commit adds another helper that does the
same but returns the sorted keys instead
Most of the support for valgrind was under HAVE_VALGRIND_VALGRIND_H, i.e. we
would enable if the valgrind headers were found. The operations then we be
conditionalized on RUNNING_UNDER_VALGRIND.
But in a few places we had code which was conditionalized on VALGRIND, i.e. the
config option. I noticed because I compiled with -Dvalgrind=true on a machine
that didn't have valgrind.h, and the build failed because
RUNNING_UNDER_VALGRIND was not defined. My first idea was to add a check that
the header is present if the option is set, but it seems better to just remove
the option. The code to support valgrind is trivial, and if we're
!RUNNING_UNDER_VALGRIND, it has negligible cost. And the case of running under
valgrind is always some special testing/debugging mode, so we should just do
those extra steps to make valgrind output cleaner. Removing the option makes
things simpler and we don't have to think if something should be covered by the
one or the other configuration bit.
I had a vague recollection that in some places we used -Dvalgrind=true not
for valgrind support, but to enable additional cleanup under other sanitizers.
But that code would fail to build without the valgrind headers anyway, so
I'm not sure if that was still used. If there are uses like that, we can
extend the condition for cleanup_pools().
This substantially reworks mempool_cleanup() so that it releases pools
with all freed tiles only, but keeps all pools with still-allocated
tiles around.
This is more correct, as the previous implementation just released all
pools regardless if anything was still used or not. This would make
valgrind shut up but would just hide memory leaks altogether. Moreover
if called during regular runtime of a program would result in bad memory
accesses all over.
Hence, let's add a proper implementation and only trim pools we really
know are empty.
This way we can safely call these functions later, when under memory
pressure, at any time.
Otherwise it complains about a set but unused variable:
```
../src/basic/hashmap.c:1070:48: error: variable 'n_rehashed' set but not used [-Werror,-Wunused-but-set-variable]
unsigned old_n_buckets, new_n_buckets, n_rehashed, new_n_entries;
^
1 error generated.
```
Before we had the following scheme:
mempool_enabled() would check mempool_use_allowed, and
libsystemd-shared would be linked with a .c file that provides mempool_use_allowed=true,
while other things would linked with a different .c file with mempool_use_allowed=false.
In the new scheme, mempool_enabled() itself is a weak symbol. If it's
not found, we assume false. So it only needs to be provided for libsystemd-shared,
where it can return false or true.
test-set-disable-mempool is libshared, so it gets the symbol. But then we
actually disable the mempool via envvar. mempool_enable() is called to check
its return value directly.
Note, if `n != SIZE_MAX`, we cannot check the existence of the specified
string in the set without duplicating the string. And, set_consume() also
checks the existence of the string. Hence, it is not necessary to call
set_contains() if `n != SIZE_MAX`.
In general we almost never hit those asserts in production code, so users see
them very rarely, if ever. But either way, we just need something that users
can pass to the developers.
We have quite a few of those asserts, and some have fairly nice messages, but
many are like "WTF?" or "???" or "unexpected something". The error that is
printed includes the file location, and function name. In almost all functions
there's at most one assert, so the function name alone is enough to identify
the failure for a developer. So we don't get much extra from the message, and
we might just as well drop them.
Dropping them makes our code a tiny bit smaller, and most importantly, improves
development experience by making it easy to insert such an assert in the code
without thinking how to phrase the argument.
Before we invoke n_entries() we need to check for non-NULL here, like in
all other calls to the helper function. Otherwise we'll crash when
invoked with a NULL object, which we usually consider equivalent to an
empty one though.
We recently started making more use of malloc_usable_size() and rely on
it (see the string_erase() story). Given that we don't really support
sytems where malloc_usable_size() cannot be trusted beyond statistics
anyway, let's go fully in and rework GREEDY_REALLOC() on top of it:
instead of passing around and maintaining the currenly allocated size
everywhere, let's just derive it automatically from
malloc_usable_size().
I am mostly after this for the simplicity this brings. It also brings
minor efficiency improvements I guess, but things become so much nicer
to look at if we can avoid these allocation size variables everywhere.
Note that the malloc_usable_size() man page says relying on it wasn't
"good programming practice", but I think it does this for reasons that
don't apply here: the greedy realloc logic specifically doesn't rely on
the returned extra size, beyond the fact that it is equal or larger than
what was requested.
(This commit was supposed to be a quick patch btw, but apparently we use
the greedy realloc stuff quite a bit across the codebase, so this ends
up touching *a*lot* of code.)
The commit 6f3ac0d517 drops the prefix and
suffix in TAGS= property. But there exists several rules that have like
`TAGS=="*:tag:*"`. So, the property must be always prefixed and suffixed
with ":".
Fixes#17930.
if we allocate a bunch of hash tables all at the same time, with none
earlier than the other, there's a good chance we'll initialize the
shared hash key multiple times, so that some threads will see a
different shared hash key than others.
Let's fix that, and make sure really everyone sees the same hash key.
Fixes: #17007
I think this is nicer in general, and here in particular we have a lot
of code like:
static inline IteratedCache* hashmap_iterated_cache_new(Hashmap *h) {
return (IteratedCache*) _hashmap_iterated_cache_new(HASHMAP_BASE(h));
}
and it's visually appealing to use the same whitespace in the function
signature and the cast in the body of the function.
The compiler would do this to, esp. with LTO, but we can short-circuit the
whole process and make everything a bit simpler by avoiding the separate
definition.
(It would be nice to do the same for _set_new(), _set_ensure_allocated()
and other similar functions which are one-line trivial wrappers too. Unfortunately
that would require enum HashmapType to be made public, which we don't want
to do.)
Also use double space before the tracking args at the end. Without
the comma this looks ugly, but it's a bit better with the double space.
At least it doesn't look like a variable with a type.
This combines set_ensure_allocated() with set_consume(). The cool thing is that
because we know the hash ops, we can correctly free the item if appropriate.
Similarly to set_consume(), the goal is to simplify handling of the case where
the item needs to be freed on error and if already present in the set.
It's such a common operation to allocate the set and put an item in it,
that it deserves a helper. set_ensure_put() has the same return values
as set_put().
Comes with tests!
"internal" is a lot of characters. Let's take a leaf out of the Python's book
and simply use _ to mean private. Much less verbose, but the meaning is just as
clear, or even more.
