-
Notifications
You must be signed in to change notification settings - Fork 282
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
Simplified slice #2674
Simplified slice #2674
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2674 +/- ##
==========================================
- Coverage 64.62% 64.61% -0.01%
==========================================
Files 104 104
Lines 22239 22237 -2
Branches 10911 10911
==========================================
- Hits 14371 14369 -2
Misses 5626 5626
Partials 2242 2242 ☔ View full report in Codecov by Sentry. |
slice.hpp needs more. There are some that don't work for some reason. |
include/exiv2/slice.hpp
Outdated
/*! | ||
* Constructs a new constant subSlice. Behaves otherwise exactly like | ||
* the non-const version. | ||
*/ | ||
[[nodiscard]] Slice<const container> subSlice(size_t begin, size_t end) const { | ||
Slice<const container> subSlice(size_t begin, size_t end) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove [[nodiscard]]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I don't, it starts warning on the unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to fix that in the unit tests. Could you suppress the warning with the cast-to-void trick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh I don't see the value. All such warnings are in ASSERT_THROW
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Fixed the unit tests.
9dba372
to
5c7bd32
Compare
f1198ad
to
1ae0f4c
Compare
5ce098e
to
1b44a48
Compare
These are the changes that I'd like to add: neheb#7 |
@kevinbackhouse fixed. Changed the void casts to static casts. |
No description provided.