Skip to content

Issue 841: replace assert with enforce. #842

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

Conversation

kevinbackhouse
Copy link
Collaborator

Fixes #841.

@kevinbackhouse kevinbackhouse force-pushed the afl_bugfixes_crwimage_data_location branch 2 times, most recently from 01f9f35 to c97ac5a Compare May 13, 2019 12:08
Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

I'm going to leave it to Dan and Luis to approve. Dan brought enforce() into the code-base.

I believe you're in Oxford. You're very welcome to come to Camberley for a BBQ during the summer. We're going to Scotland and other short trips in the next couple of months. After that, it'd be nice to meet up.

@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #842 into 0.27-maintenance will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           0.27-maintenance     #842      +/-   ##
====================================================
+ Coverage             62.71%   62.72%   +<.01%     
====================================================
  Files                   156      156              
  Lines                 21568    21566       -2     
====================================================
  Hits                  13527    13527              
+ Misses                 8041     8039       -2
Impacted Files Coverage Δ
src/crwimage_int.hpp 70% <ø> (ø) ⬆️
src/crwimage_int.cpp 39.92% <100%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dc87a1...3f1ac9f. Read the comment docs.

@D4N
Copy link
Member

D4N commented May 13, 2019

I've grep'ed the source code for invocations of dataLocation() and it is only used in very few places. What's your guys opinion on simply making it throw if it should find an invalid location?

@kevinbackhouse
Copy link
Collaborator Author

@D4N: that sounds fine to me. Would you like me to add that to this PR?

@kevinbackhouse kevinbackhouse force-pushed the afl_bugfixes_crwimage_data_location branch from c97ac5a to 1a5fa3f Compare May 13, 2019 16:00
@piponazo
Copy link
Collaborator

I think @D4N 's idea is very nice. If you could try to throw inside CiffComponent::dataLocation(uint16_t tag) then we could remove all the checks from where dataLocation() is used. Could you try to do it in this PR? If we observe complications because of that we can always postpone it.

piponazo
piponazo previously approved these changes May 13, 2019
D4N
D4N previously approved these changes May 14, 2019
Copy link
Member

@D4N D4N left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@D4N
Copy link
Member

D4N commented May 14, 2019

@kevinbackhouse Could you rebase the commints on top of 0.27-maintenance and squast them into one?

@kevinbackhouse kevinbackhouse force-pushed the afl_bugfixes_crwimage_data_location branch from d939ae3 to 7eeb638 Compare May 14, 2019 13:36
@mergify mergify bot dismissed stale reviews from piponazo and D4N May 14, 2019 13:36

Pull request has been modified.

@piponazo piponazo merged commit 7798ae2 into Exiv2:0.27-maintenance May 15, 2019
mergify bot pushed a commit that referenced this pull request May 15, 2019
piponazo pushed a commit that referenced this pull request May 15, 2019
mergify bot pushed a commit that referenced this pull request May 16, 2019
@clanmills clanmills added this to the v0.27.2 milestone May 19, 2019
@kevinbackhouse kevinbackhouse deleted the afl_bugfixes_crwimage_data_location branch September 18, 2021 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants