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

[3.x] Move COREAUDIO_ENABLED check inside AUDIO_DRIVER_COREAUDIO_H block #87271

Closed
wants to merge 1 commit into from

Conversation

halotroop2288
Copy link
Contributor

No description provided.

@halotroop2288 halotroop2288 requested a review from a team as a code owner January 16, 2024 20:10
@AThousandShips AThousandShips changed the title Fix mislabeled #endifs in CoreAudio header [3.x] Fix mislabeled #endifs in CoreAudio header Jan 16, 2024
@halotroop2288 halotroop2288 changed the title [3.x] Fix mislabeled #endifs in CoreAudio header [3.x] Move COREAUDIO_ENABLED check inside AUDIO_DRIVER_COREAUDIO_H block Jan 17, 2024
@halotroop2288
Copy link
Contributor Author

OK we can do it your way. It's done.

@akien-mga
Copy link
Member

akien-mga commented Jan 17, 2024

Thanks for the contribution!

As I pointed out on #87272 (comment), for the 3.x branch we prefer not to do this kind of style fixes unless they actually solve a problem. It took a lot of effort to improve the code style consistency of the whole codebase in the master branch, and redoing all that work for 3.x wouldn't be worth the effort at this stage.

The change in this commit is correct, but I know there are many other places with similar style inconsistencies which have no incidence on the functionality of the code, as I've fixed them all in the master branch. For this case specifically, you can have a look at #63368. As you can see, more than a thousand lines of code were impacted, so doing it file by file as started in this PR wouldn't be a good option. I wouldn't suggest trying to backport this PR to 3.x, as again this would be more effort than worth (not only to do it, but also for maintainers to review it and make sure there's effectively no change in behavior).

I included this change in #87272 because why not, but otherwise I'd advise against trying to tackle other such minor header guard name/comment inconsistencies if they're not actual bugs that change the code logic.

@akien-mga akien-mga closed this Jan 17, 2024
@AThousandShips AThousandShips removed this from the 3.x milestone Jan 17, 2024
@halotroop2288 halotroop2288 deleted the patch-2 branch January 17, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants