The alloc and nr fields of a prio-queue tell us how much memory is
allocated and used in the array. So the natural type for them is size_t,
which prevents overflow on 64-bit systems where "int" is still 32 bits.
This is unlikely to happen in practice, as we typically use it for
storing commits, and having 2^31 of those is rather a lot. But it's good
to keep our generic data structures as flexible as possible. And as we
start to enforce -Wsign-compare, it means that callers need to use
"int", too, and the problem proliferates. Let's fix it at the source.
The changes here can be put into a few groups:
1. Changing the alloc/nr fields in the struct to size_t. This requires
swapping out int for size_t in negotiator/skipping.c, as well as
in prio_queue_get(), because those all iterate over the array.
Building with -Wsign-compare complains about these.
2. Other code that assigns or passes around indexes into the array
(e.g., the swap() and compare() functions) won't trigger
-Wsign-compare because we are simply truncating the values. These
are caught by -Wconversion, but I've adjusted them here to
future-proof us.
3. In prio_queue_reverse() we compute "queue->nr - 1" without checking
if anything is in the queue, which underflows now that nr is
unsigned. We can fix that by returning early when the queue is
empty (there is nothing to reverse).
4. The insertion_ctr variable is currently unsigned, but can likewise
grow (it is actually worse, because adding and removing an element
many times will keep increasing the counter, even though "nr" does
not). I've bumped that to size_t here, as well.
But -Wconversion notes that computing the "cmp" result by
subtracting the counters and assigning to "int" is a potential
problem. And that's true even before this patch, since we use an
unsigned counter (imagine comparing "2^32-1" and "0", which should
be a high positive value, but instead is "-1" as a signed int).
Since we only care about the sign (and not the magnitude) of the
result, we could fix this by swapping out the subtraction for a
ternary comparison. Probably the performance impact would be
negligible, since we just called into a custom compare function and
branched on its result anyway. But it's easy enough to do a
branchless version by subtracting the comparison results.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There has been a push to remove extern from function declarations.
Remove some instances of "extern" for function declarations which are
caught by Coccinelle. Note that Coccinelle has some difficulty with
processing functions with `__attribute__` or varargs so some `extern`
declarations are left behind to be dealt with in a future patch.
This was the Coccinelle patch used:
@@
type T;
identifier f;
@@
- extern
T f(...);
and it was run with:
$ git ls-files \*.{c,h} |
grep -v ^compat/ |
xargs spatch --sp-file contrib/coccinelle/noextern.cocci --in-place
Files under `compat/` are intentionally excluded as some are directly
copied from external sources and we should avoid churning them as much
as possible.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When consuming a priority queue, it can be convenient to inspect
the next object that will be dequeued without actually dequeueing
it. Our existing library did not have such a 'peek' operation, so
add it as prio_queue_peek().
Add a reference-level comparison in t/helper/test-prio-queue.c
so this method is exercised by t0009-prio-queue.sh. Further, add
a test that checks the behavior when the compare function is NULL
(i.e. the queue becomes a stack).
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If two items are added to a prio_queue and compare equal,
they currently come out in an apparently random order (this
order is deterministic for a particular sequence of
insertions and removals, but does not necessarily match the
insertion order). This makes it unlike using a date-ordered
commit_list, which is one of the main types we would like to
replace with it (because prio_queue does not suffer from
O(n) insertions).
We can make the priority queue stable by keeping an
insertion counter for each element, and using it to break
ties. This does increase the memory usage of the structure
(one int per element), but in practice it does not seem to
affect runtime. A best-of-five "git rev-list --topo-order"
on linux.git showed less than 1% difference (well within the
run-to-run noise).
In an ideal world, we would offer both stable and unstable
priority queues (the latter to try to maximize performance).
However, given the lack of a measurable performance
difference, it is not worth the extra code.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Use the prio-queue data structure to implement a priority queue of
commits sorted by committer date, when handling --date-order. The
structure can also be used as a simple LIFO stack, which is a good
match for --topo-order processing.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Traditionally we used a singly linked list of commits to hold a set
of in-flight commits while traversing history. The most typical use
of the list is to add commits that are newly discovered to it, keep
the list sorted by commit timestamp, pick up the newest one from the
list, and keep digging. The cost of keeping the singly linked list
sorted is nontrivial, and this typical use pattern better matches a
priority queue.
Introduce a prio-queue structure, that can be used either as a LIFO
stack, or a priority queue. This will be used in the next patch to
hold in-flight commits during sort-in-topological-order.
Tests and the idea to make it usable for any "void *" pointers to
"things" are by Jeff King. Bugs are mine.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King