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

fix bad for range loop #1635

Merged
merged 1 commit into from
May 13, 2021
Merged

fix bad for range loop #1635

merged 1 commit into from
May 13, 2021

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented May 13, 2021

This loop is supposed to break when it encounters a match.

Signed-off-by: Rosen Penev [email protected]

I tried converting this to std::find_if. Didn't work.

This loop is supposed to break when it encounters a match.

Signed-off-by: Rosen Penev <[email protected]>
@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@091fd77). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head c458e70 differs from pull request most recent head 0970053. Consider uploading reports for the commit 0970053 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1635   +/-   ##
=======================================
  Coverage        ?   66.91%           
=======================================
  Files           ?      151           
  Lines           ?    20805           
  Branches        ?        0           
=======================================
  Hits            ?    13921           
  Misses          ?     6884           
  Partials        ?        0           

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 091fd77...0970053. Read the comment docs.

@neheb
Copy link
Collaborator Author

neheb commented May 13, 2021

Maybe the find is better.

@neheb
Copy link
Collaborator Author

neheb commented May 13, 2021

find causes address sanitizer issues. Oh well.

Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

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

Great! It looks you have fixed the tests that were failing on Windows from time to time. It is interesting to see that those tests were not failing on Linux nor on Mac.

I can backport this change to the 0.27-maintenance.

Thanks!

@piponazo
Copy link
Collaborator

Ups ... weird, in the PR view all the checks look green, but when I go to the github actions page, I see that some of these flaky tests (on BMFF code) on windows are still failing:

https://github.com/Exiv2/exiv2/runs/2573006142?check_suite_focus=true

Just for clarification. Were you trying to fix some defect in Exiv2 that was already reported?

@piponazo
Copy link
Collaborator

I was not looking to the right github actions link ... that is the one were all the windows jobs are passing:
https://github.com/Exiv2/exiv2/actions/runs/837867978

What I do not understand is that the failing tests seemed to be on BMFF while your change is in the epsimage.cpp file.

@piponazo piponazo merged commit 5dc4292 into Exiv2:main May 13, 2021
@piponazo
Copy link
Collaborator

I tried to cherry-pick the change to include it in 0.27-maintenance, but we do not need it there

@neheb neheb deleted the fr branch May 13, 2021 09:16
@neheb
Copy link
Collaborator Author

neheb commented May 13, 2021

No this was a bad transformation on my part.

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