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

Reduce number of Clang warnings #607

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gruenich
Copy link
Contributor

@gruenich gruenich commented Jan 25, 2025

Some are only part of -Wall or -Wpedantic.

MediaPriority got a major rewrite to use C++ instead of a clever switch statement that is difficult to understand.

Copy link
Member

Choose a reason for hiding this comment

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

No. Please leave my switch untouched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Is it okay that I replace the macro RETURN_IF by direct ifs with std::string::compare and returns?
  2. Should I replace the inside the macro RETURN_IF the ::strcmp by std::string::compare?
  3. Should I leave the whole MediaPriority.cc as it is, even if subjectively it is more difficult to maintain that it has to be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with 1. and removed the macro RETURN_IF but preserved the switch.

@gruenich gruenich force-pushed the feature/clang-warnings branch 2 times, most recently from f4b7d9b to 5d026c4 Compare February 6, 2025 15:49
Found by Clang (Winconsistent-missing-override)
Found by Clang (Wunused-private-field).
Found by Clang (Wunqualified-std-cast-call).
Found by Clang (Wunused-lambda-capture).
Avoids use of C macro that is discouraged by C++ Core
gGidlines. Performance should be not crucial in this case.

Fixes a warning found by Clang (Wstring-plus-int).
@gruenich gruenich force-pushed the feature/clang-warnings branch from 5d026c4 to 25f5528 Compare February 11, 2025 20:34
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.

2 participants