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 3 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
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 16 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.

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 <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
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