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

More migration to size_t #2145

Merged
merged 4 commits into from
Mar 14, 2022
Merged

More migration to size_t #2145

merged 4 commits into from
Mar 14, 2022

Conversation

piponazo
Copy link
Collaborator

Continuation of the work done to avoid some of the static_casts we have around the code base. See other recent PRs: #2118 #2109 #2062

@piponazo piponazo added the refactoring Cleanup / Simplify code -> more readable / robust label Mar 11, 2022
@lgtm-com
Copy link

lgtm-com bot commented Mar 11, 2022

This pull request introduces 6 alerts when merging b20e073 into 7aae68e - view on LGTM.com

new alerts:

  • 4 for Wrong type of arguments to formatting function
  • 1 for Unsigned comparison to zero
  • 1 for Comparison result is always the same

@piponazo piponazo marked this pull request as draft March 11, 2022 17:04
@neheb
Copy link
Collaborator

neheb commented Mar 11, 2022

Seems good at first glance. I do wonder whether or not static_cast is needlessly verbose. In my code I prefer to use functional casts, eg. size_t(x).

@piponazo
Copy link
Collaborator Author

Seems good at first glance. I do wonder whether or not static_cast is needlessly verbose. In my code I prefer to use functional casts, eg. size_t(x).

Whenever you use C++ it is better to use the C++ casts because they just do what you specify. The usage of C-style casts can be actually dangerous in some cases, because they try to do different kind of casts in a determined order. You can find more information here:
https://stackoverflow.com/questions/332030/when-should-static-cast-dynamic-cast-const-cast-and-reinterpret-cast-be-used

Anyways, as I mentioned in this PR and in others, the intention is to remove as many static_casts as possible from the codebase. They are normally a code smell indicating that a better data model could reduce the need of them.

@clanmills
Copy link
Collaborator

I like Rosen's idea to use function casts. The static cast syntax is ugly and the C cast is hackery.

@neheb
Copy link
Collaborator

neheb commented Mar 11, 2022

Well, the function cast is the same as a C cast. Just looks less ugly and avoids the C cast clang-tidy warning.

I like using it for simple types like size_t where the compiler's not really going to do anything strange.

@clanmills
Copy link
Collaborator

The equivalent of a cast in Pascal was size_t(...). When I started to work in C, I was shocked by the cast. I couldn't believe that a language designer would use anything so hideous. My first program did something with argv, and I had a function with the signature something(char*** args). It was my first program for "The IBM PC". After being used the multi-window Apollo workstation, it was quite a shock. The C tradition of vulgar syntax continued with the static_cast<voodoo> .... C with all its *'s and ->'s looks like Custer at the Little Bighorn.

I read an interview with Stroudstup and he said something like "C++20 is the language I set out the make in the first place". I wonder if it will be as nice as Pascal.

@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #2145 (4355d63) into main (173ebb2) will decrease coverage by 0.04%.
The diff coverage is 68.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2145      +/-   ##
==========================================
- Coverage   63.31%   63.27%   -0.05%     
==========================================
  Files          97       97              
  Lines       19152    19159       +7     
  Branches     9724     9721       -3     
==========================================
- Hits        12127    12123       -4     
- Misses       4762     4778      +16     
+ Partials     2263     2258       -5     
Impacted Files Coverage Δ
app/actions.hpp 100.00% <ø> (ø)
include/exiv2/exif.hpp 100.00% <ø> (ø)
include/exiv2/image.hpp 100.00% <ø> (ø)
include/exiv2/metadatum.hpp 72.72% <ø> (ø)
include/exiv2/pgfimage.hpp 100.00% <ø> (ø)
include/exiv2/xmp_exiv2.hpp 92.30% <ø> (ø)
src/cr2header_int.hpp 50.00% <ø> (ø)
src/cr2image.cpp 18.98% <0.00%> (ø)
src/makernote_int.hpp 91.66% <ø> (ø)
src/olympusmn_int.cpp 43.63% <0.00%> (ø)
... and 38 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 173ebb2...4355d63. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Mar 14, 2022

This pull request introduces 4 alerts when merging 4355d63 into 173ebb2 - view on LGTM.com

new alerts:

  • 4 for Wrong type of arguments to formatting function

@piponazo piponazo marked this pull request as ready for review March 14, 2022 11:17
@piponazo
Copy link
Collaborator Author

The PR is now ready for review after I fixed problems reported by the CI jobs.

@neheb & @clanmills , IMHO I think that the discussion about the casting style is offtopic (This PR is probably not the best place to discuss such things. I am actually mostly removing casts here). Feel free to open another issue or trigger this conversation in the team chat.

@clanmills
Copy link
Collaborator

You're quite right, @piponazo. Apologies for the noise.

@piponazo
Copy link
Collaborator Author

You're quite right, @piponazo. Apologies for the noise.

No worries, I appreciate interesting discussions in the right channels 😁

@piponazo piponazo merged commit 573c3c7 into main Mar 14, 2022
@piponazo piponazo deleted the mainSizeT_n branch March 14, 2022 20:42
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.

3 participants