more SonarLint cleanups #2531

neheb · 2023-03-04T06:05:22Z

No description provided.

ghost · 2023-03-04T06:06:59Z

👇 Click on the image for a new way to code review

Legend

codecov · 2023-03-04T06:16:07Z

Codecov Report

Merging #2531 (9a5ea32) into main (4a26eba) will decrease coverage by 0.07%.
The diff coverage is 74.25%.

❗ Current head 9a5ea32 differs from pull request most recent head 943f03f. Consider uploading reports for the commit 943f03f to get more accurate results

@@            Coverage Diff             @@
##             main    #2531      +/-   ##
==========================================
- Coverage   63.97%   63.91%   -0.07%     
==========================================
  Files         103      103              
  Lines       22467    22389      -78     
  Branches    10858    10829      -29     
==========================================
- Hits        14374    14309      -65     
+ Misses       5872     5862      -10     
+ Partials     2221     2218       -3

Impacted Files	Coverage Δ
include/exiv2/metadatum.hpp	`78.57% <ø> (-1.43%)`	⬇️
include/exiv2/riffvideo.hpp	`100.00% <ø> (ø)`
include/exiv2/slice.hpp	`91.30% <ø> (ø)`
include/exiv2/tags.hpp	`0.00% <ø> (ø)`
src/crwimage_int.hpp	`95.55% <ø> (+7.32%)`	⬆️
src/exif.cpp	`66.97% <0.00%> (+0.41%)`	⬆️
src/http.cpp	`0.00% <0.00%> (ø)`
src/jp2image.cpp	`70.72% <0.00%> (-0.07%)`	⬇️
src/makernote_int.cpp	`66.35% <0.00%> (-0.07%)`	⬇️
src/matroskavideo.cpp	`4.59% <0.00%> (ø)`
... and 48 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

neheb · 2023-03-05T03:33:31Z

@kevinbackhouse what do you think about the if constexpr change?

piponazo

In general the changes look good. I just made some comments and questions before approving

piponazo · 2023-03-06T16:48:59Z

src/sonymn_int.cpp

 }

-static auto getFocusMode2(const ExifData* metadata, uint32_t& val) {
+static uint32_t getFocusMode2(const ExifData* metadata) {


Did you check for this change if FocusMode2 value could be 0 ? In case this field can take this value, we would need to return another value to indicate an error in the operation.

No. I just assumed there are tests for this.

I agree with @piponazo: I'd prefer to keep the separate Boolean return value to indicate success on all of these functions, to avoid the potential confusion if the field contains "" or 0.

src/safe_op.hpp

src/basicio.cpp

src/convert.cpp

kevinbackhouse · 2023-03-12T11:02:40Z

@kevinbackhouse what do you think about the if constexpr change?

@neheb: That's very cool. I never saw that C++ feature before.

kevinbackhouse · 2023-03-19T11:50:34Z

src/safe_op.hpp

  }
-  return num < 0 ? -num : num;
+  return std::abs(num);


The type isn't signed here so I don't think there's any need to use std::abs.

Suggested change

return std::abs(num);

return num;

Signed-off-by: Rosen Penev <rosenp@gmail.com>

Found with google-readability-avoid-underscore-in-googletest-name Signed-off-by: Rosen Penev <rosenp@gmail.com>

Because of Impl, there's no way this can work properly. Signed-off-by: Rosen Penev <rosenp@gmail.com>

Signed-off-by: Rosen Penev <rosenp@gmail.com>

Just make it part of the template Signed-off-by: Rosen Penev <rosenp@gmail.com>

Seems to trip up MemorySanitizer on ARM32. Also applied various simplifications to the code using std::filesystem Signed-off-by: Rosen Penev <rosenp@gmail.com>

Found with: google-runtime-references Signed-off-by: Rosen Penev <rosenp@gmail.com>

Signed-off-by: Rosen Penev <rosenp@gmail.com>

neheb force-pushed the mf branch 3 times, most recently from ca82db6 to 31bbecf Compare March 5, 2023 01:44

neheb force-pushed the mf branch from 31bbecf to 58508d9 Compare March 5, 2023 21:03

piponazo requested changes Mar 6, 2023

View reviewed changes

neheb force-pushed the mf branch 2 times, most recently from 2c13910 to e99fd71 Compare March 7, 2023 21:37

neheb marked this pull request as ready for review March 7, 2023 21:48

neheb force-pushed the mf branch 18 times, most recently from c6904bb to aac5f54 Compare March 10, 2023 19:02

neheb force-pushed the mf branch 7 times, most recently from c6c90dd to 4c002da Compare March 19, 2023 02:24

kevinbackhouse reviewed Mar 19, 2023

View reviewed changes

neheb added 2 commits March 19, 2023 13:55

remove old MSVC workaround

9cf8467

Signed-off-by: Rosen Penev <rosenp@gmail.com>

remove MSVC hacks

004fc90

Signed-off-by: Rosen Penev <rosenp@gmail.com>

neheb force-pushed the mf branch 2 times, most recently from 0620d65 to 53482f3 Compare March 19, 2023 21:07

neheb added 13 commits March 19, 2023 14:31

replace enable_if with if constexpr

6c3ec48

Signed-off-by: Rosen Penev <rosenp@gmail.com>

clang-tidy: no _ in tests

7b30659

Found with google-readability-avoid-underscore-in-googletest-name Signed-off-by: Rosen Penev <rosenp@gmail.com>

get rid of Key() move operations

da425c8

Because of Impl, there's no way this can work properly. Signed-off-by: Rosen Penev <rosenp@gmail.com>

gcc: remove redundant struct

1d5c134

Signed-off-by: Rosen Penev <rosenp@gmail.com>

gcc: noexcept

63383c9

Signed-off-by: Rosen Penev <rosenp@gmail.com>

gcc: ifdef

a791fce

Signed-off-by: Rosen Penev <rosenp@gmail.com>

random fixes

5bec348

Signed-off-by: Rosen Penev <rosenp@gmail.com>

extern to static

9053ba7

Signed-off-by: Rosen Penev <rosenp@gmail.com>

MSVC warning fixes

be222a7

Signed-off-by: Rosen Penev <rosenp@gmail.com>

remove inline function

8222d81

Just make it part of the template Signed-off-by: Rosen Penev <rosenp@gmail.com>

don't assign an fs::path to a string

d751130

Seems to trip up MemorySanitizer on ARM32. Also applied various simplifications to the code using std::filesystem Signed-off-by: Rosen Penev <rosenp@gmail.com>

clang-tidy: don't use non const refs

40b5b15

Found with: google-runtime-references Signed-off-by: Rosen Penev <rosenp@gmail.com>

more SonarLint cleanups

943f03f

Signed-off-by: Rosen Penev <rosenp@gmail.com>

neheb force-pushed the mf branch from 53482f3 to 943f03f Compare March 19, 2023 21:31

kevinbackhouse approved these changes Mar 20, 2023

View reviewed changes

neheb merged commit 22b1201 into Exiv2:main Mar 20, 2023

neheb deleted the mf branch March 20, 2023 15:01

nehaljwani mentioned this pull request May 8, 2023

Is a new release happening soon? #2406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more SonarLint cleanups #2531

more SonarLint cleanups #2531

neheb commented Mar 4, 2023

ghost commented Mar 4, 2023 •

edited by ghost

Loading

Legend

codecov bot commented Mar 4, 2023 •

edited

Loading

neheb commented Mar 5, 2023

piponazo left a comment

piponazo Mar 6, 2023

neheb Mar 7, 2023

kevinbackhouse Mar 19, 2023

neheb Mar 19, 2023

kevinbackhouse commented Mar 12, 2023

kevinbackhouse Mar 19, 2023

more SonarLint cleanups #2531

more SonarLint cleanups #2531

Conversation

neheb commented Mar 4, 2023

ghost commented Mar 4, 2023 • edited by ghost Loading

Legend

codecov bot commented Mar 4, 2023 • edited Loading

Codecov Report

neheb commented Mar 5, 2023

piponazo left a comment

Choose a reason for hiding this comment

piponazo Mar 6, 2023

Choose a reason for hiding this comment

neheb Mar 7, 2023

Choose a reason for hiding this comment

kevinbackhouse Mar 19, 2023

Choose a reason for hiding this comment

neheb Mar 19, 2023

Choose a reason for hiding this comment

kevinbackhouse commented Mar 12, 2023

kevinbackhouse Mar 19, 2023

Choose a reason for hiding this comment

ghost commented Mar 4, 2023 •

edited by ghost

Loading

codecov bot commented Mar 4, 2023 •

edited

Loading