Skip to content

Add -Wsign-conversion to the list of common warning flags #2775

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

Open
wants to merge 12 commits into
base: devel
Choose a base branch
from

Conversation

Wmbat
Copy link

@Wmbat Wmbat commented Dec 6, 2023

Description

Fixes all implicit sign conversions and turns on -Wsign-conversion when building catch2. I usually tried to find a good solution to the warning instead of casting.

Ran all the tests on my machine and it seemed good to go

GitHub Issues

Related to #2583

@Wmbat Wmbat force-pushed the wmbat/fixing-implicit-conversions branch from 249b5f5 to f28ab91 Compare December 6, 2023 23:52
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Merging #2775 (f28ab91) into devel (0520ff4) will decrease coverage by 0.06%.
The diff coverage is 75.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##            devel    #2775      +/-   ##
==========================================
- Coverage   91.45%   91.39%   -0.06%     
==========================================
  Files         194      194              
  Lines        8197     8196       -1     
==========================================
- Hits         7496     7490       -6     
- Misses        701      706       +5     

@@ -297,7 +297,7 @@ class TablePrinter {
std::ostream& m_os;
std::vector<ColumnInfo> m_columnInfos;
ReusableStringStream m_oss;
int m_currentColumn = -1;
std::size_t m_currentColumn = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better if this was an iterator?

Copy link

Choose a reason for hiding this comment

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

+1

Copy link
Author

Choose a reason for hiding this comment

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

Changed

Copy link

@shahsb shahsb left a comment

Choose a reason for hiding this comment

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

This is a good improvement and will make a great impact to the project. Thanks @Wmbat for making these changes.. Good Job!!

@@ -297,7 +297,7 @@ class TablePrinter {
std::ostream& m_os;
std::vector<ColumnInfo> m_columnInfos;
ReusableStringStream m_oss;
int m_currentColumn = -1;
std::size_t m_currentColumn = 0;
Copy link

Choose a reason for hiding this comment

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

+1

@Wmbat Wmbat force-pushed the wmbat/fixing-implicit-conversions branch from f28ab91 to 1d322a4 Compare March 29, 2025 21:28
@Wmbat Wmbat requested a review from shahsb March 29, 2025 21:38
@Wmbat
Copy link
Author

Wmbat commented Mar 29, 2025

Fixed the merge conflicts and switch to using an iterator instead of an index

not fully sure why it says I modified so many files though

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.

3 participants