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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues detected with PVS-Studio + other little improvements #1689

Merged
merged 32 commits into from
Jun 1, 2021

Conversation

piponazo
Copy link
Collaborator

In this PR I am fixing several issues detected with PVS-Studio and few other issues detected while doing this work. Please review the changes commit per commit, since each one does something completely different.

https://pvs-studio.com/en

Note that I got a free PVS-Studio license for Exiv2 to my personal email account. I need to check if we can include this license to be used in CI and if other Exiv2 collaborators could use it or not. I will investigate that in the future 馃槈

@piponazo piponazo added this to the v1.00 milestone May 29, 2021
@piponazo piponazo self-assigned this May 29, 2021
@piponazo piponazo requested a review from sridharb1 as a code owner May 29, 2021 06:19
@codecov
Copy link

codecov bot commented May 29, 2021

Codecov Report

Merging #1689 (bfbfe82) into main (024830a) will decrease coverage by 0.00%.
The diff coverage is 65.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1689      +/-   ##
==========================================
- Coverage   66.90%   66.90%   -0.01%     
==========================================
  Files         151      151              
  Lines       20781    20762      -19     
==========================================
- Hits        13903    13890      -13     
+ Misses       6878     6872       -6     
Impacted Files Coverage 螖
include/exiv2/basicio.hpp 83.33% <酶> (+5.55%) 猬嗭笍
include/exiv2/bmffimage.hpp 100.00% <酶> (酶)
include/exiv2/types.hpp 90.90% <酶> (酶)
src/actions.hpp 100.00% <酶> (酶)
src/convert.cpp 69.14% <酶> (酶)
src/exiv2app.hpp 100.00% <酶> (酶)
src/jpgimage.cpp 77.66% <酶> (+0.21%) 猬嗭笍
src/preview.cpp 57.75% <0.00%> (酶)
src/tags_int.cpp 87.07% <酶> (酶)
src/tiffvisitor_int.cpp 87.51% <0.00%> (+0.23%) 猬嗭笍
... and 12 more

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 024830a...bfbfe82. Read the comment docs.

@neheb
Copy link
Collaborator

neheb commented May 29, 2021

Looks good.

hassec
hassec previously approved these changes May 29, 2021
Copy link
Member

@hassec hassec left a comment

Choose a reason for hiding this comment

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

Awesome stuff @piponazo 馃憤

I've only found a few small nitpicks, thus I'll just approve this and leave it up to you if you want to accept any of my comments 馃槈

include/exiv2/basicio.hpp Show resolved Hide resolved
src/actions.cpp Outdated Show resolved Hide resolved
src/actions.cpp Outdated Show resolved Hide resolved
samples/geotag.cpp Outdated Show resolved Hide resolved
samples/geotag.cpp Outdated Show resolved Hide resolved
sridharb1
sridharb1 previously approved these changes May 29, 2021
Copy link
Collaborator

@sridharb1 sridharb1 left a comment

Choose a reason for hiding this comment

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

Looks good on the whole. Were these changes suggested by pvs-studio as well or only the issues identified by it?

@piponazo piponazo dismissed stale reviews from sridharb1 and hassec via cb99031 May 29, 2021 14:56
@piponazo
Copy link
Collaborator Author

Looks good on the whole. Were these changes suggested by pvs-studio as well or only the issues identified by it?

Most of the issues were identified via PVS-Studio and I wrote a suffix in the commit message. Although I might have forgotten to mark some of the commits with that prefix

Copy link
Member

@hassec hassec left a comment

Choose a reason for hiding this comment

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

LGTM 馃憤

@piponazo piponazo merged commit f30022d into main Jun 1, 2021
@piponazo piponazo deleted the main-SmallImprovements branch June 1, 2021 10:39
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.

None yet

5 participants