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

switch from custom stringFormat to fmtlib #2769

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented Sep 17, 2023

The latter helps to avoid wrong format errors and is simpler to use. Will be replaced by std::format once C++20 becomes mandatory.

@ghost
Copy link

ghost commented Sep 17, 2023

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

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@codecov
Copy link

codecov bot commented Sep 17, 2023

Codecov Report

Attention: Patch coverage is 36.36364% with 35 lines in your changes are missing coverage. Please review.

Project coverage is 64.57%. Comparing base (77915ad) to head (a0aa4a7).
Report is 11 commits behind head on main.

❗ Current head a0aa4a7 differs from pull request most recent head 51fd65b. Consider uploading reports for the commit 51fd65b to get more accurate results

Files Patch % Lines
src/rafimage.cpp 0.00% 26 Missing ⚠️
src/basicio.cpp 14.28% 6 Missing ⚠️
src/bmffimage.cpp 71.42% 2 Missing ⚠️
src/image.cpp 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2769      +/-   ##
==========================================
- Coverage   64.62%   64.57%   -0.05%     
==========================================
  Files         104      104              
  Lines       22239    22155      -84     
  Branches    10911    10849      -62     
==========================================
- Hits        14371    14306      -65     
+ Misses       5626     5610      -16     
+ Partials     2242     2239       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kmilos
Copy link
Collaborator

kmilos commented Oct 18, 2023

How about we just wait for C++20 as min requirement and use std::format straight away? Or code it in a way to use std::format if using C++20 so there is one less dependency?

CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
src/basicio.cpp Outdated Show resolved Hide resolved
@neheb neheb force-pushed the fmt branch 7 times, most recently from 1ac41a6 to db8dbda Compare October 20, 2023 18:46
CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
src/image_int.hpp Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
src/image_int.hpp Outdated Show resolved Hide resolved
src/rafimage.cpp Outdated Show resolved Hide resolved
@neheb neheb force-pushed the fmt branch 9 times, most recently from 43e31a2 to 51b29fe Compare November 16, 2023 21:38
@neheb neheb force-pushed the fmt branch 6 times, most recently from e204e8b to 2efbdf3 Compare November 17, 2023 23:40
@neheb
Copy link
Collaborator Author

neheb commented Nov 17, 2023

remaining error needs to be fixed by @kevinbackhouse I think.

kmilos
kmilos previously approved these changes Nov 27, 2023
@neheb
Copy link
Collaborator Author

neheb commented Feb 16, 2024

Rebased.

@mergify mergify bot dismissed kmilos’s stale review February 16, 2024 02:42

Pull request has been modified.

@kmilos kmilos added the refactoring Cleanup / Simplify code -> more readable / robust label Jul 3, 2024
@neheb
Copy link
Collaborator Author

neheb commented Jul 9, 2024

Rebased.

@neheb
Copy link
Collaborator Author

neheb commented Dec 20, 2024

Rebased.

@kmilos
Copy link
Collaborator

kmilos commented Dec 20, 2024

@neheb I think I managed to install GCC 13 for the Cygwin job so this can finally proceed.

@@ -126,6 +127,27 @@ jobs:
meson setup "${{github.workspace}}/build" -Dauto_features=${{matrix.deps}} -Dwarning_level=3 -Dcpp_std=c++20
meson compile -C "${{github.workspace}}/build" --verbose
meson test -C "${{github.workspace}}/build" --verbose
Cygwin:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this can be removed if we're happy w/ the other Cygwin action now that could be reverted. Although MSYS is somewhat equivalent, no one really uses it to build/ship exiv2 AFAIK...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use MSYS here as it’s less verbose than Cygwin. Plus meson has its own subprojects.

@neheb
Copy link
Collaborator Author

neheb commented Dec 20, 2024

Hmm? Not sure what you mean by GCC 13 on Cygwin needed.

@kmilos
Copy link
Collaborator

kmilos commented Dec 20, 2024

Hmm? Not sure what you mean by GCC 13 on Cygwin needed.

It's been a while, heh? 😉 There is no fmt package on Cygwin, and only GCC (libstdc++) 13 onwards has this feature.

@kevinbackhouse
Copy link
Collaborator

I think the CIFuzz error might fix itself after this is merged. My guess is that OSS-Fuzz is running ci/install_dependencies.sh from the main branch, so the fmt package isn't installed. If something goes wrong, we can do a follow-up PR to fix it.

The latter helps to avoid wrong format errors and is simpler to use.
Will be replaced by std::format once C++20 becomes mandatory.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
@neheb
Copy link
Collaborator Author

neheb commented Dec 21, 2024

hmmm CMAKE_CXX_STANDARD is not being overwritten.

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.

4 participants