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

Replace Value::toLong with Value::toInt64. #2062

Merged
merged 14 commits into from
Feb 12, 2022

Conversation

kevinbackhouse
Copy link
Collaborator

The type long is problematic because it's 32 bits on Windows and 64 bits on Linux. I would like to start replacing it. In some cases, it will make sense to replace it with size_t, in others something like int64_t will be the best choice. In this PR, I have replaced Value::toLong() with Value::toInt64(). Value is used for values that were read from the image file, so I think a platform-independent type is the most appropriate replacement here. I also added Value::toUint32() for convenience.

@kevinbackhouse kevinbackhouse added this to the v1.00 milestone Jan 31, 2022
@kevinbackhouse kevinbackhouse added the refactoring Cleanup / Simplify code -> more readable / robust label Jan 31, 2022
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #2062 (ccc4fd6) into main (a8a995f) will decrease coverage by 0.23%.
The diff coverage is 63.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2062      +/-   ##
==========================================
- Coverage   62.08%   61.85%   -0.24%     
==========================================
  Files          96       96              
  Lines       19153    19110      -43     
  Branches     9798     9781      -17     
==========================================
- Hits        11891    11820      -71     
- Misses       4967     5012      +45     
+ Partials     2295     2278      -17     
Impacted Files Coverage Δ
include/exiv2/types.hpp 70.58% <ø> (ø)
src/casiomn_int.cpp 7.14% <0.00%> (ø)
src/exif.cpp 66.01% <0.00%> (ø)
src/iptc.cpp 64.20% <0.00%> (ø)
src/properties.cpp 73.95% <ø> (ø)
src/samsungmn_int.cpp 6.45% <0.00%> (ø)
src/tiffimage_int.cpp 79.32% <0.00%> (-0.12%) ⬇️
src/xmp.cpp 77.28% <0.00%> (ø)
src/minoltamn_int.cpp 51.05% <5.88%> (ø)
src/olympusmn_int.cpp 17.72% <13.33%> (ø)
... and 29 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 a8a995f...ccc4fd6. Read the comment docs.

@kevinbackhouse kevinbackhouse marked this pull request as draft January 31, 2022 11:20
@@ -1274,6 +1298,61 @@ namespace Exiv2 {
ValueList value_;

private:
//! Utility for toInt64, toUint32, etc.
template<typename I>
inline I float_to_integer_helper(long n) const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to codecov, many of these new lines are not covered by tests. Did you check if this new code is covered? I know that codecov sometimes report false issues.

@piponazo
Copy link
Collaborator

piponazo commented Jan 31, 2022

In general I like the idea. I also want to do the same thing in the BasicIO classes, where we are using long in many places where it makes more sense to use something like std::size_t. Because of that the codebase is plaged with static_casts in many places, and I think we can reduce those static casts considerably.

@kevinbackhouse
Copy link
Collaborator Author

I found the reason why tests/bugfixes/redmine/test_issue_876.py was failing on the MSYS2 platform. It was caused by a type mismatch between this:

exiv2/src/tags_int.hpp

Lines 230 to 236 in a8a995f

struct TagDetails {
int64_t val_; //!< Tag value
const char* label_; //!< Translation of the tag value
//! Comparison operator for use with the find template
bool operator==(long key) const { return val_ == key; }
}; // struct TagDetails

and this TagDetails initializer:

{ static_cast<long int>(0x80000302), "EOS 6D" },

@piponazo
Copy link
Collaborator

Ey @kevinbackhouse , is this ready for review or is it still a Draft?

@kevinbackhouse kevinbackhouse marked this pull request as ready for review February 12, 2022 17:54
@kevinbackhouse
Copy link
Collaborator Author

@piponazo: yes, it's ready for review now. I rebased in the hope that it would fix the PVS check, but it didn't help unfortunately.

We should probably do a "squash and merge" on this so that it's just one commit, but I have left it as separate commits so that you can see what I did to fix the msys2 problem.

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.

It looks good to me 👍

Regarding the squashing, as you want. Feel free to merge it with the squash option. I normally choose to squash or not depending on the relevance of the individual commits.

@kevinbackhouse kevinbackhouse merged commit 2a3dd2d into Exiv2:main Feb 12, 2022
@kevinbackhouse kevinbackhouse deleted the ValueToLong branch February 12, 2022 21:16
@StefanBruens
Copy link

This is somewhat asymmetric now, as TypeId only has (un)signedLong (which explicitly says it is 32bit, and 64bit is `(un)signedLongLong).

enum TypeId {
unsignedByte = 1, //!< Exif BYTE type, 8-bit unsigned integer.
asciiString = 2, //!< Exif ASCII type, 8-bit byte.
unsignedShort = 3, //!< Exif SHORT type, 16-bit (2-byte) unsigned integer.
unsignedLong = 4, //!< Exif LONG type, 32-bit (4-byte) unsigned integer.

This also changes the API.

@kmilos
Copy link
Collaborator

kmilos commented May 19, 2023

@StefanBruens Don't think that's a problem as those are TIFF types, not Exiv2 API types - TIFF 32-bit LONG/SLONG is easily contained by int64_t.

However, thanks for pointing out we might have a potential problem w/ LONG8/IFD8 and might need uint64_t versions as well (i.e. Value::toUint64()). Note that Exiv2 doesn't support BigTIFF yet anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Cleanup / Simplify code -> more readable / robust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants