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

basic conversion to C++20 #2645

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

basic conversion to C++20 #2645

wants to merge 4 commits into from

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented Jun 5, 2023

No description provided.

@ghost
Copy link

ghost commented Jun 5, 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 Jun 5, 2023

Codecov Report

Attention: Patch coverage is 69.23077% with 28 lines in your changes missing coverage. Please review.

Project coverage is 64.64%. Comparing base (77915ad) to head (e7676de).
Report is 59 commits behind head on main.

Files with missing lines Patch % Lines
src/epsimage.cpp 0.00% 17 Missing ⚠️
src/sonymn_int.cpp 73.68% 0 Missing and 5 partials ⚠️
src/pentaxmn_int.cpp 0.00% 0 Missing and 4 partials ⚠️
src/pgfimage.cpp 80.00% 1 Missing ⚠️
src/tiffcomposite_int.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2645      +/-   ##
==========================================
+ Coverage   64.62%   64.64%   +0.02%     
==========================================
  Files         104      104              
  Lines       22239    22248       +9     
  Branches    10911    10921      +10     
==========================================
+ Hits        14371    14383      +12     
+ Misses       5626     5624       -2     
+ Partials     2242     2241       -1     
Flag Coverage Δ
64.64% <69.23%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@neheb neheb force-pushed the h branch 3 times, most recently from e68ae8a to 4cddaa6 Compare June 6, 2023 11:20
@neheb
Copy link
Collaborator Author

neheb commented Jun 7, 2023

Hrm I’m unsure of this PR. Compiler minimums have to increase for everything.

@mandree
Copy link
Contributor

mandree commented Jul 1, 2023

Hrm I’m unsure of this PR. Compiler minimums have to increase for everything.

For library code it is usually good to use newer standards only in the implementation, but not in the interface. However I do not know how to map this in Windows or whenever something links to features you need a newer standard C++/template library because then people still need to upgrade. So the safe way would be to make the library stuff compatible from whatever minimum C++ standard version is desired (11/14/17) but pushing all users to C++20 might make exiv2 unsuitable for many of those who use it today.

@neheb
Copy link
Collaborator Author

neheb commented Jul 1, 2023

Right. The headers are still kept at C++11. On that note, if constexpr should be removed from them as compilers are not free to add it implicitly as I originally thought, making the templates much less efficient.

As far as Windows is concerned, that's not an issue. In the case of MSVC, the libraries are upgrade-able, I actually don't know if older MSVC can even compile exiv2. I remember MSVC2019 or something just crashing when trying to compile exiv2. No errors printed.

On that note, there's currently an issue where Exiv2 is incompatible with msvcrt. Given that it's deprecated by Microsoft, I see no value in supporting it. See #2637

@kmilos
Copy link
Collaborator

kmilos commented Jul 13, 2023

Btw, might be an idea to hold off on this for a couple of 0.28.x bugfix releases, just so it remains easy to backport stuff from the main branch?

@neheb
Copy link
Collaborator Author

neheb commented Jul 13, 2023

This is still draft after all.

@neheb
Copy link
Collaborator Author

neheb commented Jul 19, 2023

Yeah I'm not sure of this PR anymore. People are still using GCC7 to compile Exiv2. std::endian comes with GCC8 and starts_with GCC9. Let's not mention other projects that are on C++14 to allow building with CentOS7. This is going to be a draft for a while....

template <size_t N, const char* const (&keys)[N]>
ExifData::const_iterator findMetadatum(const ExifData& ed) {
for (const auto& k : keys) {
auto pos = ed.findKey(ExifKey(k));

Check warning

Code scanning / CodeQL

NULL iterator deref Warning

Iterator returned by findKey might cause a null deref
here
.
Iterator returned by findKey might cause a null deref
here
.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wrong.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
18.04 is now EOL. 20.04 comes with 9 standard. Allows building with
C++20.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
There's std::endian now.

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

kmilos commented Sep 28, 2024

Let's not mention other projects that are on C++14 to allow building with CentOS7.

CentOS 7 is EOL now btw. I think a bump in requirements is not totally unreasonable, that would be coming from RHEL 8, Debian 11 and Ubuntu 20.04 currently, so GCC 9 should be doable for most...

@mandree
Copy link
Contributor

mandree commented Sep 28, 2024

Kindly do not pay too much attention on the decrepit Ubuntu LTS versions, that LTS promise holds only for "main"-series packages but not for "universe" what the average desktop is constituted from, so just ignore old Ubuntu. No upstream projects has to cater for museum software on the desktops... is software as Exiv2 normally (unless on a rolling release) really rebuilt frequently by end users?

Or will the new software only propagate into distros briefly before time their release slush/feature freeze? In the latter case, look what the newest stable of your favorite distros has in C++ toolchain and libs, not the oldest. Lowest common denominator slows you down by half a decade or so. And C++23 has been voted on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants