1 #2212
1 #2212
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2212 +/- ##
=======================================
Coverage 63.30% 63.30%
=======================================
Files 116 116
Lines 19597 19599 +2
Branches 9545 9545
=======================================
+ Hits 12405 12407 +2
Misses 5117 5117
Partials 2075 2075
Continue to review full report at Codecov.
|
neheb
force-pushed
the
1
branch
4 times, most recently
from
e81909c
to
d14d14f
Compare
There was a problem hiding this comment.
2nd commit is nice 👏
In the 1st one, I do not have a strong opinion about removing the const keyword in parameters passed by value. However, with variables passed by pointer, I do not like to have different signatures in the header/cpp files. I think it might trigger unnecessary mental load for the code reader.
| @@ -47,16 +47,16 @@ struct EXIV2API Photoshop { | |||
| /// @return 0 if successful;<BR> | |||
| /// 3 if no data for psTag was found in pPsData;<BR> | |||
| /// -2 if the pPsData buffer does not contain valid data. | |||
| static int locateIrb(const byte* pPsData, size_t sizePsData, uint16_t psTag, const byte** record, | |||
There was a problem hiding this comment.
Two comments here:
- Why to change the signature only in the header and not in the .cpp too?
- After looking at the code, I think it would even make more sense to pass the 2 last parameters by reference instead of by pointer. Then we do not need to check if those variables are
nullptror not.
| @@ -622,7 +622,7 @@ TiffComponent* newCasio2Mn2(uint16_t tag, IfdId group, IfdId mnGroup); | |||
| @param pRoot Pointer to the root component of the TIFF tree. | |||
| @return An index into the array set, -1 if no match was found. | |||
| */ | |||
| int sonyCsSelector(uint16_t tag, const byte* pData, size_t size, TiffComponent* const pRoot); | |||
There was a problem hiding this comment.
Same comment in all these functions insider makernote_int. I feel unconfortable having different signatures in the declaration and definitions. It might add unnecessary complexity for the code reader without much benefit.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Found with readability-avoid-const-params-in-decls Signed-off-by: Rosen Penev <rosenp@gmail.com>
There's no nullptr here. Signed-off-by: Rosen Penev <rosenp@gmail.com>
std::move is used. Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
No description provided.