Upgrade C++ standard to c++17 #2052

piponazo · 2022-01-07T17:22:05Z

As discussed in #2051, we thought it would be a good idea to start using C++17 so that we can use some new available features to simplify our codebase.

codecov · 2022-01-07T17:29:49Z

Codecov Report

Merging #2052 (aa3b21c) into main (825c0c7) will decrease coverage by 55.76%.
The diff coverage is n/a.

❗ Current head aa3b21c differs from pull request most recent head 64da72a. Consider uploading reports for the commit 64da72a to get more accurate results

@@            Coverage Diff             @@
##             main   #2052       +/-   ##
==========================================
- Coverage   61.48%   5.72%   -55.77%     
==========================================
  Files          96      96               
  Lines       19192   18940      -252     
  Branches     9842    9840        -2     
==========================================
- Hits        11801    1085    -10716     
- Misses       5077   17667    +12590     
+ Partials     2314     188     -2126

Impacted Files	Coverage Δ
src/jpgimage.cpp	`0.00% <ø> (-70.49%)`	⬇️
src/getopt.hpp	`0.00% <0.00%> (-100.00%)`	⬇️
src/actions.hpp	`0.00% <0.00%> (-100.00%)`	⬇️
src/exiv2app.hpp	`0.00% <0.00%> (-100.00%)`	⬇️
src/fujimn_int.cpp	`0.00% <0.00%> (-100.00%)`	⬇️
src/orfimage_int.hpp	`0.00% <0.00%> (-100.00%)`	⬇️
src/rw2image_int.hpp	`0.00% <0.00%> (-100.00%)`	⬇️
include/exiv2/exif.hpp	`0.00% <0.00%> (-100.00%)`	⬇️
include/exiv2/iptc.hpp	`0.00% <0.00%> (-100.00%)`	⬇️
include/exiv2/tags.hpp	`0.00% <0.00%> (-100.00%)`	⬇️
... and 80 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 825c0c7...64da72a. Read the comment docs.

piponazo · 2022-01-07T17:33:50Z

I need to also fix the compilation with clang + samples:

[ 55%] Building CXX object samples/CMakeFiles/exiv2json.dir/exiv2json.cpp.o
In file included from /src/exiv2/samples/exiv2json.cpp:24:
/src/exiv2/samples/Jzon.h:211:32: error: 'iterator<std::input_iterator_tag, std::pair<std::string, Jzon::Node &>>' is deprecated [-Werror,-Wdeprecated-declarations]
                class iterator : public std::iterator<std::input_iterator_tag, NamedNode>
                                             ^
/usr/local/bin/../include/c++/v1/__iterator/iterator.h:24:29: note: 'iterator<std::input_iterator_tag, std::pair<std::string, Jzon::Node &>>' has been explicitly marked deprecated here
struct _LIBCPP_TEMPLATE_VIS _LIBCPP_DEPRECATED_IN_CXX17 iterator

lgtm-com · 2022-01-07T17:38:27Z

This pull request fixes 1 alert when merging 0a1e08c into 825c0c7 - view on LGTM.com

fixed alerts:

1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

lgtm-com · 2022-01-07T18:43:58Z

This pull request fixes 7 alerts when merging 4871afe into 825c0c7 - view on LGTM.com

fixed alerts:

7 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

piponazo · 2022-01-07T19:05:39Z

CMakeLists.txt

@@ -32,7 +32,7 @@ option( EXIV2_ENABLE_WEBREADY         "Build webready support into library"
 option( EXIV2_ENABLE_CURL             "USE Libcurl for HttpIo (WEBREADY)"                     OFF )
 option( EXIV2_ENABLE_BMFF             "Build with BMFF support"                               ON  )

-option( EXIV2_BUILD_SAMPLES           "Build sample applications"                             ON  )
+option( EXIV2_BUILD_SAMPLES           "Build sample applications"                             OFF )


I had to disable that because the workflow .github/workflows/cifuzz.yml was failing to compile the project with clang14 (due to the issue with the Jzon class + C++17).

lgtm-com · 2022-01-07T19:06:18Z

This pull request fixes 7 alerts when merging 7f1b7c7 into 825c0c7 - view on LGTM.com

fixed alerts:

7 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

neheb · 2022-01-07T20:36:54Z

‘’’
_LIBCPP_ENABLE_CXX17_REMOVED_FEATURES
‘’’

Needs to be defined for clang.

neheb · 2022-01-07T20:38:48Z

include/exiv2/jpgimage.hpp

-        static const char* const bimId_;                 //!< %Photoshop IRB marker (deprecated)
-        static const uint16_t iptc_;    //!< %Photoshop IPTC marker
-        static const uint16_t preview_; //!< %Photoshop preview marker
+        inline static const char* const ps3Id_ = "Photoshop 3.0\0"; //!< %Photoshop marker


AFAIK inline here is redundant. static implies inline. This should also be constexpr.

Maybe even std::string_view. Although I assume that would break API.

I do not think static implies inline. The inline specified for static variable members is something new in C++17:
https://en.cppreference.com/w/cpp/language/inline

I just did this change because the previous code was failing to compile with C++17.

it's constexpr that implies inline.

what compile error?

I just checked it out. It was actually not a compiler error but linking one:

-- Build files have been written to: /media/linuxDev/programming/exiv2/buildRelease [134/183] Linking CXX executable bin/key-test FAILED: bin/key-test : && ccache /usr/bin/c++ -O3 -DNDEBUG -fstack-protector-strong samples/CMakeFiles/key-test.dir/key-test.cpp.o -o bin/key-test -Wl,-rpath,/media/linuxDev/programming/exiv2/buildRelease/lib lib/libexiv2.so.1.0.0.9 /home/luis/.conan/data/zlib/1.2.11/_/_/package/6af9cc7cb931c5ad942174fd7838eb655717c709/lib/libz.a && : /usr/bin/ld: lib/libexiv2.so.1.0.0.9: undefined reference to `Exiv2::Photoshop::irbId_'

I think the problem is that in the header file it was declared as:

static const std::array<const char*, 4> irbId_;

While in the cpp file:

constexpr std::array<const char*, 4> Photoshop::irbId_

I could also declare it as constexpr in the header file as you suggested so that we do not need to add the inline specifier.

No that error has to do with Windows and dllexport. defining them in the header is the correct solution.

Small nitpick: static constexpr vs. constexpr static.

I actually got the error on Ubuntu 20.4 with Gcc-10 😅 . I'll change the order to static constexpr

I just realized. C++17 supports CTAD. You can safely ommit the template parameters.

neheb · 2022-01-07T20:52:16Z

samples/Jzon.cpp

@@ -25,6 +25,10 @@ THE SOFTWARE.
 #define JzonAPI __declspec(dllexport)
 #endif

+#ifdef WIN32


This should be _MSC_VER or something. Not relevant for mingw.

Speaking of this it would probably be a good idea to set up an msys CI

The CI does build MinGW/msys2. We don't have that in the nightly GHA build. Would you like to change/fix that?

We actually have that MSYS2 build which is triggered on PRs:
https://github.com/Exiv2/exiv2/actions/runs/1670765792

I think it is enough to have it there and not in the nightly builds.

Now that I think about it, on Windows + MSYS2, it is ending up using MinGW (with gcc-11). The problem with the Jzon class is only reproducible with VisualStudio and very recent versions of clang.

piponazo · 2022-01-08T07:40:28Z

‘’’ _LIBCPP_ENABLE_CXX17_REMOVED_FEATURES ‘’’

Needs to be defined for clang.

I added this and enable back the SAMPLES=ON , so that the CIFuzz job could run with the samples, but the problem persist:

https://github.com/Exiv2/exiv2/runs/4746935159?check_suite_focus=true

It looks like that definition for clang does not generate the expected behavior.

lgtm-com · 2022-01-08T07:45:54Z

This pull request fixes 1 alert when merging 6b73b7e into 825c0c7 - view on LGTM.com

fixed alerts:

1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

lgtm-com · 2022-01-08T08:06:27Z

This pull request fixes 7 alerts when merging d39767c into 825c0c7 - view on LGTM.com

fixed alerts:

7 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

neheb · 2022-01-08T08:49:53Z

maybe it needs to be set earlier. who knows.

edit: maybe set _LIBCPP_ENABLE_CXX17_REMOVED_FEATURES in Jzon.h

piponazo · 2022-01-08T10:08:24Z

maybe it needs to be set earlier. who knows.

edit: maybe set _LIBCPP_ENABLE_CXX17_REMOVED_FEATURES in Jzon.h

I would not like to spend more time on that Jzon class at the moment. Yesterday I triggered a discussion about how useful that Jzon class is and the sample where it is used (exiv2json). If we end up trowing away the Jzon class (and even the exiv2json sample), all the efforts spend on that code would be useless.

lgtm-com · 2022-01-08T10:09:01Z

This pull request fixes 7 alerts when merging 64da72a into 825c0c7 - view on LGTM.com

fixed alerts:

7 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

clanmills · 2022-01-08T10:34:50Z

@piponazo If the JSON code is causing you trouble with MSVC, then don't build it on that platform. It'll be something like this in samples/CMakeLists.txt

if ( NOT MSVC )
    add_executable(          exiv2json exiv2json.cpp Jzon.cpp Jzon.h)
    list(APPEND APPLICATIONS exiv2json)
endif()

I'm sure CMake knows the version of Visual Studio, so you can probably continue to build it on 2017 and 2019.

piponazo · 2022-01-09T09:18:24Z

@piponazo If the JSON code is causing you trouble with MSVC, then don't build it on that platform

I just needed to add a compiler definition in the Jzon.cpp file to surpass that error. I did not want to spend much time in that class since it is part of a sample app and not part of the exiv2 library.

Right now the work in this branch is done and we could merge it as it is (when somebody approves the PR).

clanmills · 2022-01-09T09:52:28Z

I'm happy to approve that, although I don't know C++17. However @neheb has commented, so I believe the code is good.

clanmills · 2022-01-09T09:53:56Z

Let's adopt the convention that the person how opens a PR almost merges it.

piponazo added 2 commits January 7, 2022 17:55

Enable C++17 and use new inline specifier for const variable members

6ed38db

Silence deprecation warning on MSVC

0a1e08c

piponazo requested review from clanmills, neheb, kevinbackhouse, kmilos and hassec January 7, 2022 17:22

piponazo force-pushed the main_cpp17 branch from 510c5e4 to 4871afe Compare January 7, 2022 18:31

Disable samples by default

7f1b7c7

piponazo force-pushed the main_cpp17 branch from 4871afe to 7f1b7c7 Compare January 7, 2022 18:51

piponazo commented Jan 7, 2022

View reviewed changes

neheb reviewed Jan 7, 2022

View reviewed changes

piponazo force-pushed the main_cpp17 branch from 6b73b7e to d39767c Compare January 8, 2022 07:48

improvements after review

64da72a

piponazo force-pushed the main_cpp17 branch from d39767c to 64da72a Compare January 8, 2022 09:57

clanmills approved these changes Jan 9, 2022

View reviewed changes

kevinbackhouse approved these changes Jan 9, 2022

View reviewed changes

piponazo merged commit b0318c3 into main Jan 9, 2022

piponazo deleted the main_cpp17 branch January 9, 2022 19:09

nehaljwani mentioned this pull request May 8, 2023

Is a new release happening soon? #2406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade C++ standard to c++17 #2052

Upgrade C++ standard to c++17 #2052

piponazo commented Jan 7, 2022

codecov bot commented Jan 7, 2022 •

edited

Loading

piponazo commented Jan 7, 2022

lgtm-com bot commented Jan 7, 2022

lgtm-com bot commented Jan 7, 2022

piponazo Jan 7, 2022

lgtm-com bot commented Jan 7, 2022

neheb commented Jan 7, 2022

neheb Jan 7, 2022

neheb Jan 7, 2022

piponazo Jan 8, 2022

neheb Jan 8, 2022

piponazo Jan 8, 2022

neheb Jan 8, 2022

piponazo Jan 8, 2022

neheb Jan 8, 2022

neheb Jan 7, 2022

neheb Jan 7, 2022

clanmills Jan 8, 2022

piponazo Jan 8, 2022

piponazo commented Jan 8, 2022

lgtm-com bot commented Jan 8, 2022

lgtm-com bot commented Jan 8, 2022

neheb commented Jan 8, 2022 •

edited

Loading

piponazo commented Jan 8, 2022

lgtm-com bot commented Jan 8, 2022

clanmills commented Jan 8, 2022

piponazo commented Jan 9, 2022

clanmills commented Jan 9, 2022

clanmills commented Jan 9, 2022

Upgrade C++ standard to c++17 #2052

Upgrade C++ standard to c++17 #2052

Conversation

piponazo commented Jan 7, 2022

codecov bot commented Jan 7, 2022 • edited Loading

Codecov Report

piponazo commented Jan 7, 2022

lgtm-com bot commented Jan 7, 2022

lgtm-com bot commented Jan 7, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Jan 7, 2022

neheb commented Jan 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piponazo commented Jan 8, 2022

lgtm-com bot commented Jan 8, 2022

lgtm-com bot commented Jan 8, 2022

neheb commented Jan 8, 2022 • edited Loading

piponazo commented Jan 8, 2022

lgtm-com bot commented Jan 8, 2022

clanmills commented Jan 8, 2022

piponazo commented Jan 9, 2022

clanmills commented Jan 9, 2022

clanmills commented Jan 9, 2022

codecov bot commented Jan 7, 2022 •

edited

Loading

neheb commented Jan 8, 2022 •

edited

Loading