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

C++11 compatibility for headers #2257

Merged
merged 2 commits into from
Jul 24, 2022
Merged

C++11 compatibility for headers #2257

merged 2 commits into from
Jul 24, 2022

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented Jul 2, 2022

I'll put this as a draft.

I tested compilation with darktable and it seems that compilation worked.

OTOH, when I look at some of the other stuff in the headers like std::remove_cv_t, that gets introduced in C++14. I need a way to test C++11 compilation of all the headers.

@neheb
Copy link
Collaborator Author

neheb commented Jul 2, 2022

those C++14 changes are quite verbose...

@codecov
Copy link

codecov bot commented Jul 2, 2022

Codecov Report

Merging #2257 (c304808) into main (75c53ea) will increase coverage by 0.00%.
The diff coverage is 40.90%.

@@           Coverage Diff           @@
##             main    #2257   +/-   ##
=======================================
  Coverage   63.46%   63.46%           
=======================================
  Files         118      118           
  Lines       19598    19599    +1     
  Branches     9556     9556           
=======================================
+ Hits        12438    12439    +1     
  Misses       5094     5094           
  Partials     2066     2066           
Impacted Files Coverage Δ
include/exiv2/basicio.hpp 91.66% <ø> (ø)
include/exiv2/bmffimage.hpp 100.00% <ø> (ø)
include/exiv2/bmpimage.hpp 100.00% <ø> (ø)
include/exiv2/cr2image.hpp 100.00% <ø> (ø)
include/exiv2/epsimage.hpp 0.00% <ø> (ø)
include/exiv2/gifimage.hpp 100.00% <ø> (ø)
include/exiv2/image.hpp 100.00% <ø> (ø)
include/exiv2/jp2image.hpp 100.00% <ø> (ø)
include/exiv2/mrwimage.hpp 100.00% <ø> (ø)
include/exiv2/orfimage.hpp 100.00% <ø> (ø)
... and 26 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 75c53ea...c304808. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2022

This pull request fixes 1 alert when merging 52d7ea6 into 8a9f6ac - view on LGTM.com

fixed alerts:

  • 1 for Self comparison

@neheb
Copy link
Collaborator Author

neheb commented Jul 2, 2022

That alert is not fixed as much as it is hidden honestly.

@piponazo
Copy link
Collaborator

piponazo commented Jul 3, 2022

Hi @neheb . Why are you proposing this change? Did the team recently have a discussion about using C++11 in our API? As far as I remember, in the team chat few people voted to choose C++17 for the development in main.

Update: I saw now #2255 where the discussion is going on

@neheb
Copy link
Collaborator Author

neheb commented Jul 5, 2022

@piponazo I tested with

c++ -DEXIV2API='' -std=c++11 *.hpp

and no more C++17 warnings.

[[nodiscard]]

is interesting though. It gets introduced in C++17, that's true. But the C++ standard says that unknown attributes should not be an error. No idea if this should be handled.

@lgtm-com
Copy link

lgtm-com bot commented Jul 5, 2022

This pull request fixes 1 alert when merging 6312e04 into 8a9f6ac - view on LGTM.com

fixed alerts:

  • 1 for Self comparison

@neheb neheb marked this pull request as ready for review July 8, 2022 00:58
neheb added 2 commits July 7, 2022 17:58
This should allow usage with C++11 projects. It's also wrong. The only
user of this assigns an std::string from a string_view, which is not
safe.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
Remove is_signed_v and CTAD for std::array as they are in C++17

Remove remove_cv_t, remove_pointer_t, and make_unsigned_t as they are in
C++14

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

lgtm-com bot commented Jul 8, 2022

This pull request fixes 1 alert when merging c304808 into 75c53ea - view on LGTM.com

fixed alerts:

  • 1 for Self comparison

Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

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

The changes look good to me. The only things missing is the automatic method to test. It would be nice to have a mechanism to detect automatically when somebody place new code in the headers which is not C++11 compliant. But we can take care of that in a different PR.

@neheb neheb merged commit 02b0ff3 into Exiv2:main Jul 24, 2022
@neheb neheb deleted the 33 branch July 24, 2022 23:37
@kevinbackhouse kevinbackhouse linked an issue Feb 22, 2023 that may be closed by this pull request
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.

Can we keep public headers C++11 compatible?
2 participants