Re: [Rapp-dev] SSSE3 alignment and rapp_validate_alignment

[Hans-Peter Nilsson]

> From: Willie Betschart <address@hidden>
> Date: Wed, 19 Feb 2014 11:17:53 +0100

> I deliver two patches to RAPP

Note for the future: separate messages are preferred for
separate issues.

(Also, the test-case could be a separate patch since it tests
existing functions.  The rapp_validate_* patch could then build
on that.)

> Correct-alignment-for-SSSE3.patch sets an alignment of 16 bytes.

Oops.  How did we not see this before?  Thanks.

Though this bug is in the original code set, this also
highlights a porting-pitfall: you have to tweak this code too,
for a new port, which is easily overlooked and may have a
somewhat hairy failure mode.

> Tested with
> ./configure --enable-backend=ssse3
> make check

I'm curious: have you observed it failing?
Was it from the new test or was it "just" code inspection?

> rapp_validate_alignment.patch

> This adds a new API, mainly for validating buffers allocated
> form external sources, i.e. OpenCV buffers.

...or rather, buffers from e.g. drivers...

> Example 
> if(rapp_validate_alignment(buf, ....) == 0){
>    new_buf = rapp_malloc()
>    copy(new_buf, buf)
>    free(buf)
>    buf = new_buf
> }

Beware of the bug in that example: you need to hold on to the
old buffer when it can't be free'd with "rapp_free()".

At first I thought "why, what's the benefit of this function
compared to (((uintptr_t) buf) & (rapp_alignment - 1))" but then
I looked at the patch and noticed that the function *does* have
value on its own, but is misnamed; it validates not only the
alignment but also the easily-overlooked buffer parameters (also
should be pointed out in its doxygen comments).  Thus, I think
it would be better named "rapp_validate_buffer"; the parameter
checking is of value for buffers built from rapp_malloc too.

The patch itself has issues in using a different formatting
style than other RAPP code, for example missing space before
if-argument parentheses and the code-block opening bracket (see
your code example); comment formatting (should also be full
sentences) and for using "//" for comments.

Patches should always follow the existing coding conventions.
There are other issues, please take care.  As for any project,
double-check with nearby code and for this project we even have
it in a file called HACKING.  And the test-suite header seems
odd; it tests several things (which is nice), not just
rapp_malloc.  As mentioned, that could go in separately.

I'm on the fence here: the API would change, but we don't add
any new core function.  Though it'd be a nice convenience, all
the functionality is already there; the user "just" has to
remember to check not just the buffer pointer (with an
expression like the one above) but also the "dimensions" of the
buffer.  So, I think the function should go in only together
with other new functionality that affects the API, to avoid a
version bump just for a convenience function.

Thanks.

brgds, H-P