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 _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss