Replace Metadatum::toLong() with Metadatum::toInt64(). #2107
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2107 +/- ##
==========================================
+ Coverage 63.75% 63.80% +0.05%
==========================================
Files 96 96
Lines 19202 19171 -31
Branches 9798 9772 -26
==========================================
- Hits 12242 12232 -10
Misses 4658 4658
+ Partials 2302 2281 -21
Continue to review full report at Codecov.
|
|
It looks good to me so far. I'll happily approve when you think the PR is "Ready for review" |
kevinbackhouse
force-pushed
the
MetadataToLong
branch
from
4f75ab9
to
da07aa8
Compare
src/preview.cpp
Outdated
| @@ -506,8 +506,8 @@ namespace { | |||
| return false; | |||
| image->readMetadata(); | |||
|
|
|||
| width_ = image->pixelWidth(); | |||
| height_ = image->pixelHeight(); | |||
| width_ = static_cast<uint32_t>(image->pixelWidth()); | |||
There was a problem hiding this comment.
Hum ... I am a bit skeptical about this change. I noticed that in your second commit (76d4748) you changed pixelWidth and pixelHeight from int to int64_t, not only in the function members but also in the class members Image::pixelWidth_ and Image::pixelHeight_.
What's the rationale for addressing these changes? Maybe you should change also this height_ and width_ at in the preview files ?
In your first commit (2563658) I noticed that after making the change you could get rid of some static_casts. It is always good to get rid of them, their presence signal that something is not modeled correctly.
However in the last commits I noticed that you had to add few static_casts here and there. This make me a bit nervous ... unless you have a good reason for it.
There was a problem hiding this comment.
You're right, I've been lazy here: swapping int with int64_t in the header was the easiest way to switch it to a platform-independent type. But uint32_t is probably a more appropriate choice for pixelWidth and pixelHeight.
I'll convert this PR back to draft and work on it a bit more.
There was a problem hiding this comment.
I added Metadatum::toUint32() so that the code doesn't need to do any static casts. The implementation of Metadatum::toUint32() currently just does a static_cast but we could easily add overflow checking in the future if we want to.
kevinbackhouse
force-pushed
the
MetadataToLong
branch
from
c665f0e
to
5db9a25
Compare
kevinbackhouse
added
the
refactoring
Follow-up to #2062: replace
longin Metadatum with platform-independentint64_t.