[pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec

[Tanu Kaskinen]

On Tue, 2018-09-04 at 11:44 +0300, Tanu Kaskinen wrote:
> On Sat, 2018-07-28 at 17:34 +0200, Pali Rohár wrote:
> > +static size_t pa_sbc_select_configuration(const pa_sample_spec *sample_spec, const uint8_t *capabilities_buffer, size_t capabilities_size, uint8_t *config_buffer, size_t config_size) {
> > +    a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
> > +    a2dp_sbc_t *capabilities = (a2dp_sbc_t *) capabilities_buffer;
> 
> This looks likely to cause alignment errors, since the data in the
> uint8_t arrays doesn't have any kind of alignment guarantees. However,
> a2dp_sbc_t happens to only contain uint8_t values, so the values can't
> be badly aligned. It would be good to have a comment that reassures the
> reader that there won't be any alignment errors. Suggestion: "We cast a
> byte array to struct here, which can often cause alignment errors, but
> in this case it's safe, because a2dp_sbc_t contains only single-byte
> values."

a2dp_aptx_t has variables that are bigger than just one byte, so I
thought that we're going to have alignment issues with it. However, I
found out now that packed structs don't have alignment restrictions,
because the compiler will always be extra careful when dealing with
them. Therefore a better comment would be: "We cast a byte array to
struct here, which can often cause alignment errors, but in this case
it's safe, because a2dp_sbc_t uses the packed attribute, which makes
the compiler extra careful."

Copying that comment everywhere where we do the casting may not be such
a great idea after all, though. So much repetition. Maybe just add to
the struct definition a comment that explains what the packed attribute
does.

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk