Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed order of definitions in i2s_stream to be compatible with cpp. (AUD-5329) #1192

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tank104
Copy link

@tank104 tank104 commented Apr 8, 2024

Changed order of definitions in i2s_stream to be compatible with c++.
C++ requires define of struct to match the order.

We are getting errors like:

components/audio_stream/include/i2s_stream.h:232:1: error: designator order for field 'i2s_stream_cfg_t::transmit_mode' does not match declaration order in 'i2s_stream_cfg_t'

See issue: #1190

@CLAassistant
Copy link

CLAassistant commented Apr 8, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title Changed order of definitions in i2s_stream to be compatible with cpp. Changed order of definitions in i2s_stream to be compatible with cpp. (AUD-5329) Apr 8, 2024
@X-Ryl669
Copy link

I think those macros should die (see comment in the linked issue). You're correcting one issue here (and thank you), but many other are pending underneath for the other structures you haven't used yet. This technique of construction by macro is dumb and should be avoided. The compiler is telling you so too, just listen to it.

@tank104
Copy link
Author

tank104 commented Apr 15, 2024

Sure I agree - but up to you if you want to put it in - it does solve my issues though, and the samples.

@awong-dev
Copy link

Would love to see this merged. It's bugging me too. Either way, this fix should go in.

As for macro based construction... is there an alternate pattern for C?

From a pure compat perspective, maybe the ADF team should adopt a pattern that any macro initializer list has to have a unittest with a .cc extension so it is run through the C++ compiler. That would likely lock in the ordering during CI in a "good enough" way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants