[PATCH v3] malloc: Improve memalign alignment

[Wilco Dijkstra]

Hi DJ,

> This really needs a comment.  Only glibc defines the zero return value
> (gcc et al say "undefined"), and I'm guessing from the docs that it only
> returns zero if the alignment is higher than 0x8000...0000.  If so it
> would be more obvious to do *that* comparison.

I've added a comment and changed it to do the SIZE_MAX / 2 + 1 comparison
since we don't need to care about performance here.

> This is technically correct, but I wonder if its purpose would be more
> obvious like this?
>
>    size_t idx = TCACHE_SMALL_BINS
>                + (stdc_bit_width (nb)
>                   - stdc_bit_width (MAX_TCACHE_SMALL_SIZE));

I've changed it so it first calculates the bitwidth, then adds TCACHE_SMALL_BINS.

>> +      alignment = (size_t) 2 << (stdc_bit_width (alignment) - 1);
>
> We're using std_bit_ceil elsewhere, why not here?

We could indeed since it is off the critical path. It just didn't start out that way
in previous proposed patches.

>>     implementation.  Valid alignments for the current implementation
>> -   are non-negative powers of two.  */
>> +   are powers of two.  */
>
> We do not support negative alignments.  Either this comment didn't mean
> to you what it meant to me, or you added support for negative
> alignments?

The word negative does not make any sense in this context. I've simplified
the comment further.

> I know we don't support sizeof(void*) not being a power of 2, but if
> you're going to do this replacement, the comment needs to say that we're
> making that assumption in order to simplify the logic.

I've tweaked the comment to make it clear size of pointers is only 4 or 8.

> You need to do this as a new test in that file, not modifying an old
> test.  The existing test tests a normal case (alignment of 0x100, no
> rounding needed); your change is testing the "rounding" of the
> alignment, which is a separate (new) test.

I've added an extra testcase in v4.

Cheers,
Wilco