Skip to content

audio: mfcc: fix signed 1-bit field#9385

Merged
kv2019i merged 1 commit intothesofproject:mainfrom
cujomalainey:sparse
Aug 27, 2024
Merged

audio: mfcc: fix signed 1-bit field#9385
kv2019i merged 1 commit intothesofproject:mainfrom
cujomalainey:sparse

Conversation

@cujomalainey
Copy link
Contributor

Sparse complained about this since signed fields should be >=2 bits minimum.

lyakh
lyakh previously approved these changes Aug 22, 2024
@lyakh lyakh dismissed their stale review August 22, 2024 13:17

change of mind, prefer proper bool

int waiting_fill:1; /**< booleans */
int prev_samples_valid:1;
unsigned int waiting_fill:1; /**< booleans */
unsigned int prev_samples_valid:1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's just make them bool to avoid bit-operations. This will waste 4 bytes (unless the compiler decides to make them different size) but will be prettier, atomic, and avoid issues like the present one

lyakh
lyakh previously requested changes Aug 22, 2024
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make bool, and #9390 solves it too

@singalsu
Copy link
Collaborator

let's make bool, and #9390 solves it too

No more, I just left out this fix from there. I didn't notice that this was done before.

@cujomalainey cujomalainey requested a review from lyakh August 22, 2024 20:36
@marc-hb marc-hb dismissed lyakh’s stale review August 23, 2024 00:42

suggestion applied exactly

Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marc-hb misunderstood me, I probably didn't explain well enough: I propose to make these pure bool with no bit-counts. Just

	bool waiting_fill; /**< booleans */
	bool prev_samples_valid;

The 2 big disadvantages that I see with these 1-bit flags are (1) non-atomicity, (2) decreased speed. Sorry, @cujomalainey , I'll dare request this change again, but if there's an otherwise general consensus that this is ok, feel free to dismiss it.

Sparse complained about this since signed fields should be >=2 bits
minimum.

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
@kv2019i kv2019i merged commit 876a371 into thesofproject:main Aug 27, 2024
@cujomalainey cujomalainey deleted the sparse branch August 28, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants