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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Jan 9, 2022
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ed38db
Enable C++17 and use new inline specifier for const variable members
piponazo Jan 7, 2022
0a1e08c
Silence deprecation warning on MSVC
piponazo Jan 7, 2022
7f1b7c7
Disable samples by default
piponazo Jan 7, 2022
64da72a
improvements after review
piponazo Jan 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions .github/workflows/on_PR_linux_fuzz.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,24 @@ jobs:
steps:
- uses: actions/checkout@v2
- name: install dependencies
run: sudo ./ci/install_dependencies.sh
run: |
sudo ./ci/install_dependencies.sh
sudo apt-get install ninja-build
- name: build and compile
run: |
mkdir build && cd build
cmake -DEXIV2_ENABLE_PNG=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_CXX_COMPILER=$(which clang++) -DEXIV2_BUILD_FUZZ_TESTS=ON -DEXIV2_TEAM_USE_SANITIZERS=ON ..
make -j $(nproc)
mkdir build && cd build && \
cmake -GNinja -DEXIV2_ENABLE_PNG=ON \
-DEXIV2_BUILD_SAMPLES=ON \
-DEXIV2_ENABLE_WEBREADY=ON \
-DEXIV2_ENABLE_CURL=ON \
-DEXIV2_ENABLE_BMFF=ON \
-DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON \
-DCMAKE_CXX_COMPILER=$(which clang++) \
-DEXIV2_BUILD_FUZZ_TESTS=ON \
-DEXIV2_TEAM_USE_SANITIZERS=ON \
.. && \
cmake --build .
- name: Fuzz
run: |
Expand Down
15 changes: 13 additions & 2 deletions .github/workflows/on_PR_linux_matrix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,19 @@ jobs:
- name: Build
run: |
cd build
cmake -GNinja -DCMAKE_BUILD_TYPE=${{matrix.build_type}} -DBUILD_SHARED_LIBS=${{matrix.shared_libraries}} -DEXIV2_ENABLE_PNG=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install ..
cd build && \
cmake -GNinja \
-DCMAKE_BUILD_TYPE=${{matrix.build_type}} \
-DBUILD_SHARED_LIBS=${{matrix.shared_libraries}} \
-DEXIV2_BUILD_SAMPLES=ON \
-DEXIV2_ENABLE_PNG=ON \
-DEXIV2_ENABLE_WEBREADY=ON \
-DEXIV2_ENABLE_CURL=ON \
-DEXIV2_BUILD_UNIT_TESTS=ON \
-DEXIV2_ENABLE_BMFF=ON \
-DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON \
-DCMAKE_INSTALL_PREFIX=install \
.. && \
cmake --build .
- name: Install
Expand Down
34 changes: 30 additions & 4 deletions .github/workflows/on_PR_linux_special_buils.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,21 @@ jobs:
- name: Build
run: |
cd build
cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DEXIV2_ENABLE_PNG=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DBUILD_WITH_COVERAGE=OFF -DEXIV2_TEAM_USE_SANITIZERS=ON -DCMAKE_INSTALL_PREFIX=install ..
cd build && \
cmake -GNinja \
-DCMAKE_BUILD_TYPE=Release \
-DBUILD_SHARED_LIBS=ON \
-DEXIV2_BUILD_SAMPLES=ON \
-DEXIV2_ENABLE_PNG=ON \
-DEXIV2_ENABLE_WEBREADY=ON \
-DEXIV2_ENABLE_CURL=ON \
-DEXIV2_BUILD_UNIT_TESTS=ON \
-DEXIV2_ENABLE_BMFF=ON \
-DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON \
-DBUILD_WITH_COVERAGE=OFF \
-DEXIV2_TEAM_USE_SANITIZERS=ON \
-DCMAKE_INSTALL_PREFIX=install \
.. && \
cmake --build .
- name: Tests
Expand Down Expand Up @@ -150,8 +163,21 @@ jobs:
- name: Build
run: |
cd build
cmake -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DEXIV2_ENABLE_PNG=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DBUILD_WITH_COVERAGE=ON -DEXIV2_BUILD_DOC=ON -DEXIV2_ENABLE_NLS=ON -DCMAKE_CXX_FLAGS="-DEXIV2_DEBUG_MESSAGES" ..
cd build && \
cmake -DCMAKE_BUILD_TYPE=Release \
-DBUILD_SHARED_LIBS=ON \
-DEXIV2_BUILD_SAMPLES=ON \
-DEXIV2_ENABLE_PNG=ON \
-DEXIV2_ENABLE_WEBREADY=ON \
-DEXIV2_ENABLE_CURL=ON \
-DEXIV2_BUILD_UNIT_TESTS=ON \
-DEXIV2_ENABLE_BMFF=ON \
-DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON \
-DBUILD_WITH_COVERAGE=ON \
-DEXIV2_BUILD_DOC=ON \
-DEXIV2_ENABLE_NLS=ON \
-DCMAKE_CXX_FLAGS="-DEXIV2_DEBUG_MESSAGES" \
.. && \
make -j
- name: Generate documentation
Expand Down
16 changes: 14 additions & 2 deletions .github/workflows/on_PR_mac_matrix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,20 @@ jobs:
- name: Build
run: |
mkdir build && cd build
cmake -GNinja -DCMAKE_BUILD_TYPE=${{matrix.build_type}} -DBUILD_SHARED_LIBS=${{matrix.shared_libraries}} -DEXIV2_ENABLE_PNG=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install -DCMAKE_CXX_FLAGS="-Wno-deprecated-declarations" ..
mkdir build && cd build && \
cmake -GNinja \
-DCMAKE_BUILD_TYPE=${{matrix.build_type}} \
-DBUILD_SHARED_LIBS=${{matrix.shared_libraries}} \
-DEXIV2_BUILD_SAMPLES=ON \
-DEXIV2_ENABLE_PNG=ON \
-DEXIV2_ENABLE_WEBREADY=ON \
-DEXIV2_ENABLE_CURL=ON \
-DEXIV2_BUILD_UNIT_TESTS=ON \
-DEXIV2_ENABLE_BMFF=ON \
-DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON \
-DCMAKE_INSTALL_PREFIX=install \
-DCMAKE_CXX_FLAGS="-Wno-deprecated-declarations" \
.. && \
cmake --build .
- name: Install
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/on_PR_windows_matrix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ jobs:
-DCMAKE_CXX_FLAGS=-Wno-deprecated \
-DCMAKE_BUILD_TYPE=${{matrix.build_type}} \
-DBUILD_SHARED_LIBS=${{matrix.shared_libraries}} \
-DEXIV2_BUILD_SAMPLES=ON \
-DEXIV2_ENABLE_NLS=ON \
-DEXIV2_ENABLE_WIN_UNICODE=ON \
-DEXIV2_ENABLE_WEBREADY=ON \
Expand Down
48 changes: 41 additions & 7 deletions .github/workflows/on_push_BasicWinLinMac.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,20 @@ jobs:
- name: Build
run: |
cd build
cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DEXIV2_ENABLE_NLS=OFF -DEXIV2_ENABLE_PNG=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_WIN_UNICODE=OFF -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install ..
cmake --build .
cmake -GNinja `
-DCMAKE_BUILD_TYPE=Release `
-DBUILD_SHARED_LIBS=ON `
-DEXIV2_BUILD_SAMPLES=ON `
-DEXIV2_ENABLE_NLS=OFF `
-DEXIV2_ENABLE_PNG=ON `
-DEXIV2_ENABLE_WEBREADY=ON `
-DEXIV2_ENABLE_BMFF=ON `
-DEXIV2_BUILD_UNIT_TESTS=ON `
-DEXIV2_ENABLE_WIN_UNICODE=OFF `
-DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON `
-DCMAKE_INSTALL_PREFIX=install .. `
-S . -B build && `
cmake --build build
- name: Test
Expand Down Expand Up @@ -76,8 +87,19 @@ jobs:
- name: build and compile
run: |
cd build
cmake -GNinja -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON ..
cd build && \
cmake -GNinja \
-DCMAKE_BUILD_TYPE=Release \
-DBUILD_SHARED_LIBS=ON \
-DEXIV2_BUILD_SAMPLES=ON \
-DEXIV2_ENABLE_PNG=ON \
-DEXIV2_ENABLE_WEBREADY=ON \
-DEXIV2_ENABLE_CURL=ON \
-DEXIV2_BUILD_UNIT_TESTS=ON \
-DEXIV2_ENABLE_BMFF=ON \
-DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON \
-DCMAKE_INSTALL_PREFIX=install \
.. && \
cmake --build .
- name: Test
Expand Down Expand Up @@ -106,8 +128,20 @@ jobs:
- name: build and compile
run: |
mkdir build && cd build
cmake -GNinja -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DCMAKE_CXX_FLAGS="-Wno-deprecated-declarations" ..
mkdir build && cd build && \
cmake -GNinja \
-DCMAKE_BUILD_TYPE=Release \
-DBUILD_SHARED_LIBS=ON \
-DEXIV2_BUILD_SAMPLES=ON \
-DEXIV2_ENABLE_PNG=ON \
-DEXIV2_ENABLE_WEBREADY=ON \
-DEXIV2_ENABLE_CURL=ON \
-DEXIV2_BUILD_UNIT_TESTS=ON \
-DEXIV2_ENABLE_BMFF=ON \
-DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON \
-DCMAKE_INSTALL_PREFIX=install \
-DCMAKE_CXX_FLAGS="-Wno-deprecated-declarations" \
.. && \
cmake --build .
- name: Test
Expand Down
15 changes: 13 additions & 2 deletions .github/workflows/on_push_ExtraJobsForMain.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,19 @@ jobs:
- name: Build
run: |
cd build
cmake -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=ON -DEXIV2_ENABLE_PNG=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DBUILD_WITH_COVERAGE=ON -DCMAKE_INSTALL_PREFIX=install ..
cd build && \
cmake -DCMAKE_BUILD_TYPE=Debug \
-DBUILD_SHARED_LIBS=ON \
-DEXIV2_BUILD_SAMPLES=ON \
-DEXIV2_ENABLE_PNG=ON \
-DEXIV2_ENABLE_WEBREADY=ON \
-DEXIV2_ENABLE_CURL=ON \
-DEXIV2_BUILD_UNIT_TESTS=ON \
-DEXIV2_ENABLE_BMFF=ON \
-DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON \
-DBUILD_WITH_COVERAGE=ON \
-DCMAKE_INSTALL_PREFIX=install \
.. && \
make -j
- name: Tests + Upload coverage
Expand Down
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
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 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).

option( EXIV2_BUILD_EXIV2_COMMAND "Build exiv2 command-line executable" ON )
option( EXIV2_BUILD_UNIT_TESTS "Build unit tests" OFF )
option( EXIV2_BUILD_FUZZ_TESTS "Build fuzz tests (libFuzzer)" OFF )
Expand Down
8 changes: 2 additions & 6 deletions cmake/compilerFlags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
# These flags applies to exiv2lib, the applications, and to the xmp code
include(CheckCXXCompilerFlag)

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
if (CYGWIN)
set(CMAKE_CXX_EXTENSIONS ON)
else()
set(CMAKE_CXX_EXTENSIONS OFF)
endif()
set(CMAKE_CXX_EXTENSIONS ON)

if ( MINGW OR UNIX OR MSYS ) # MINGW, Linux, APPLE, CYGWIN
if (${CMAKE_CXX_COMPILER_ID} STREQUAL GNU)
Expand Down
10 changes: 5 additions & 5 deletions include/exiv2/jpgimage.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ namespace Exiv2 {
*/
struct EXIV2API Photoshop {
// Todo: Public for now
static const char* const ps3Id_; //!< %Photoshop marker
static const std::array<const char*, 4> irbId_; //!< %Photoshop IRB markers
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's constexpr that implies inline.

what compile error?

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 actually got the error on Ubuntu 20.4 with Gcc-10 😅 . I'll change the order to static constexpr

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

inline static const std::array<const char*, 4> irbId_{"8BIM", "AgHg", "DCSR", "PHUT"}; //!< %Photoshop IRB markers
inline static const char* const bimId_ = "8BIM"; //!< %Photoshop IRB marker (deprecated)
inline static const uint16_t iptc_ = 0x0404; //!< %Photoshop IPTC marker
inline static const uint16_t preview_ = 0x040c; //!< %Photoshop preview marker

/*!
@brief Checks an IRB
Expand Down
4 changes: 4 additions & 0 deletions samples/Jzon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ THE SOFTWARE.
#define JzonAPI __declspec(dllexport)
#endif

#ifdef WIN32
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

#define _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS
#endif

#include "Jzon.h"

#include <algorithm>
Expand Down
5 changes: 0 additions & 5 deletions src/jpgimage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,6 @@ namespace Exiv2 {
constexpr const char* JpegBase::xmpId_ = "http://ns.adobe.com/xap/1.0/\0";
constexpr const char* JpegBase::iccId_ = "ICC_PROFILE\0";

constexpr const char* Photoshop::ps3Id_ = "Photoshop 3.0\0";
constexpr std::array<const char*, 4> Photoshop::irbId_{"8BIM", "AgHg", "DCSR", "PHUT"};
constexpr const char* Photoshop::bimId_ = "8BIM"; // deprecated
constexpr uint16_t Photoshop::iptc_ = 0x0404;
constexpr uint16_t Photoshop::preview_ = 0x040c;

static inline bool inRange(int lo,int value, int hi)
{
Expand Down