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

Another cleanup round in main #2163

Merged
merged 7 commits into from
Mar 25, 2022
Merged

Another cleanup round in main #2163

merged 7 commits into from
Mar 25, 2022

Conversation

piponazo
Copy link
Collaborator

This is another round of cleanups I applied to the codebase. Each commit has different topics and is self-isolated. The commits that could raise more discussions are:

  • Usage of EXIT_SUCCESS & EXIT_FAILURE. I always use these in my projects as I think they make the code more readable.
  • Removal of calls to assert. I have a strong positioning against the usage of assert in production code, as such code would only be relevant in debug builds. Any check that is worth to be added in production should be present in all kind of builds, and for that we have enforce in our codebase. Specially ugly were the assert(false); statements ... those ones simply highlight that certain paths of the code can be considered as "dead-code". If at any point we would add a test covering those areas, the tests would simply fail in debug mode.

@piponazo piponazo added the refactoring Cleanup / Simplify code -> more readable / robust label Mar 23, 2022
@piponazo piponazo self-assigned this Mar 23, 2022
@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #2163 (313842c) into main (8ffaaab) will increase coverage by 0.59%.
The diff coverage is 74.46%.

❗ Current head 313842c differs from pull request most recent head d7f35e3. Consider uploading reports for the commit d7f35e3 to get more accurate results

@@            Coverage Diff             @@
##             main    #2163      +/-   ##
==========================================
+ Coverage   62.65%   63.25%   +0.59%     
==========================================
  Files          97       98       +1     
  Lines       19721    19581     -140     
  Branches     9710     9552     -158     
==========================================
+ Hits        12357    12386      +29     
+ Misses       5132     5122      -10     
+ Partials     2232     2073     -159     
Impacted Files Coverage Δ
app/actions.hpp 100.00% <ø> (ø)
app/exiv2app.hpp 100.00% <ø> (ø)
include/exiv2/error.hpp 47.05% <ø> (ø)
include/exiv2/types.hpp 78.72% <ø> (+1.97%) ⬆️
include/exiv2/value.hpp 84.51% <ø> (ø)
src/basicio.cpp 51.44% <0.00%> (+1.14%) ⬆️
src/crwimage.cpp 70.51% <ø> (+1.76%) ⬆️
src/error.cpp 83.87% <0.00%> (ø)
src/mrwimage.cpp 46.57% <0.00%> (ø)
src/orfimage_int.cpp 32.25% <0.00%> (ø)
... and 34 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 5ed9fb4...d7f35e3. Read the comment docs.

neheb
neheb previously approved these changes Mar 23, 2022
@neheb
Copy link
Collaborator

neheb commented Mar 23, 2022

 D:\a\exiv2\exiv2\src\bmffimage.cpp(509): error C2220: the following warning is treated as an error
D:\a\exiv2\exiv2\src\bmffimage.cpp(509): warning C4244: 'argument': conversion from 'uint64_t' to 'size_t', possible loss of data
D:\a\exiv2\exiv2\src\bmffimage.cpp(532): warning C4244: 'argument': conversion from 'uint64_t' to 'size_t', possible loss of data
D:\a\exiv2\exiv2\src\bmffimage.cpp(533): warning C4244: 'argument': conversion from 'uint64_t' to 'size_t', possible loss of data
D:\a\exiv2\exiv2\src\bmffimage.cpp(534): warning C4244: 'argument': conversion from 'uint64_t' to 'size_t', possible loss of data

I like the removal of asserts. Pointless especially with smart pointer usages.

std::copy_n can also help with removal of some memcpy usages.

@piponazo
Copy link
Collaborator Author

There are still some CI jobs failing for Windows x86. I'll try to fix those issues during the day.

@piponazo
Copy link
Collaborator Author

@neheb I merged without requesting your re-approval, since I just had to fix some casting warnings.

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.

2 participants