On Wed, Apr 21, 2021 at 11:58 PM Daniel Shahaf <d...@daniel.shahaf.name> wrote: > > + partial_result = apr_palloc(result_pool, sizeof(partial_result)); > > s/sizeof(foo)/sizeof(*foo)/, here and later in the function. > > I'm surprised that no one caught this in post-commit reviews. In fact, > I wonder whether people have reviewed this commit and missed the bug, or > didn't review this commit at all — the latter being a silent failure > mode of the commit-then-review paradigm. (More on this under separate > cover — currently on private@, but may move here later.)
(snip) Regarding counter_func(): > I'm not sure I follow what's happening here, and there aren't any > comment breadcrumbs either. I cannot grok this function either. It needs either comments to explain the intent here, or the test should be changed entirely, because I think (from my vague partial understanding) that the test is flawed. However, before jumping to conclusions, I'd like to wait for Stefan^2 to enlighten us. Meanwhile, since these 5 allocations are obviously wrong, I fixed them in r1889114 to prevent them from being forgotten there indefinitely. > test_counting() passes even if I change the > RHS's of both assignments to «*partial_result» to random values. > (test_cancellation() does fail in that case.) It looks to me (again, based on a vague partial understanding) that the test execution will self-adjust for whatever you assign here, as long as you assign a non-negative integer. If you change the RHS to a negative value, the test will (I believe) run for a very long time, and if you assign -1 to both cases or 1 to the first and -1 to the second, I think it will run forever. Two additional (very minor) issues with r1888589: The log message makes no mention of the new file subversion/tests/libsvn_subr/task-test.c, and that file starts with a spurious blank line. (It also had some trailing whitespace on various lines, but I inadvertently removed those in r1889114, much to my chagrin.) Cheers, Nathan
