Fix c++11 0.27 #1194

clanmills · 2020-05-02T11:35:35Z

This is a "quick hack" to provide support for C++11 in Exiv2 v0.27.3. If the review is favourable, this can go into v0.27.3 RC2.

A "proper" fix is to port #601 to 0.27-maintenance and I'm willing to do that if we ever do Exiv2 v0.27.4. I'm unwilling to make such a major change to the code for 0.27.3.

I don't know if we need to do further "dots". The API for v0.28 is different and applications will have to adjust to build with v0.28. And we have removed features (video, ssh and eps) from 0.28.

I feel the way forward is to focus on getting 0.28 published later in 2020.

By the way, there are changes in this PR relating to samples/geotag.cpp. Those are irrelevant and should be on another branch. Git strikes again.

…ag.cpp

piponazo · 2020-05-02T13:04:58Z

cmake/compilerFlags.cmake

@@ -1,5 +1,9 @@
 # These flags applies to exiv2lib, the applications, and to the xmp code

+if ( EXIV2_BUILD_USE_C++11 ) 


I think we wound need to following. Replace these lines by:

if ( EXIV2_BUILD_USE_C++11 ) set(CMAKE_CXX_STANDARD 11) else() set(CMAKE_CXX_STANDARD 98) endif() set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS ON)

And more important is to remove all the usages of:

target_compile_features(XXX PRIVATE cxx_std_98)

piponazo · 2020-05-02T13:07:02Z

cmake/compilerFlags.cmake

@@ -90,6 +89,10 @@ if ( MINGW OR UNIX OR MSYS ) # MINGW, Linux, APPLE, CYGWIN
            endif()
        endif()
    endif()
+    if ( EXIV2_BUILD_USE_C++11 ) 
+        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated")


Please, replace the usage of CMAKE_CXX_FLAGS by the more appropriate usage of add_compile_options:

add_compile_options(-Wno-deprecated)

I've removed that 100%. I don't mess with the compile_options for -Wno-deprecated The user can pass that from the command when required. And that's what I've documented in README.md.

piponazo · 2020-05-02T13:08:28Z

include/exiv2/config.h

+  #include <unistd.h>
+  template <typename T>
+  using auto_ptr = std::unique_ptr<T>;
+#else


That's a clever trick, I did not think about that.

I think you can remove lines 103 & 104 from here. We do not need the using instruction when not using C++11.

I can't remember what using does. I got the code from StackOverflow (in the comment).

piponazo · 2020-05-02T13:09:22Z

src/CMakeLists.txt

@@ -91,10 +91,6 @@ generate_export_header(exiv2lib
    STATIC_DEFINE exiv2lib_STATIC
 )

-target_compile_features(exiv2lib_int PRIVATE cxx_std_98)


There are some other usages of target_compile_features in the unitTests and samples folders.

I've got rid of all the target_compile_features magic.

…E_CXX_STANDARD=98

clanmills · 2020-05-02T14:17:30Z

Thanks for the review @piponazo I believe I've removed every target_compile_features(... cxx_std_98)

1100 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ find . -name *.cmake | xargs grep 98
1101 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ find . -name CMakeLists.txt | xargs grep 98
1102 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ find . -name *.cmake | xargs grep target_compile_features
1103 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ find . -name CMakeLists.txt | xargs grep target_compile_features
1104 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $

I now understand what Mr Bruins was shouting about. We shouldn't try to dictate the tools. We only have to document how to use them! So we don't need any special magic about USE_C++11. The correct way to do this is documented in README.md as:

2.17 Building with C++11 and other compilers

Exiv2 uses the default compiler for your system. Exiv2 v0.27 was written to the C++ 1998 standard and uses auto_ptr. The C++11 and C++14 compilers will issue deprecation warnings about auto_ptr. As auto_ptr support has been removed from C++17, you cannot build Exiv2 v0.27 with C++17 or later compilers. Exiv2 v0.28 and later do not use auto_ptr and will build with all compilers.

To generate a build with C++11:

$ cd <exiv2dir>
$ mkdir build ; cd build
$ cmake .. -DCMAKE_CXX_STANDARD=11 -DCMAKE_CXX_FLAGS=-Wno-deprecated
$ make

The option -DCMAKE_CXX_FLAGS=-Wno-deprecated suppresses warnings from C++11 concerning auto_ptr and other deprecated features.

clanmills · 2020-05-02T14:21:17Z

This has been very useful. It shines a clear light on our release strategy from here.

We should only do maintenance on 0.27-maintenance. So, we leave the auto_ptr alone. It is what it is.

We should port new features/fixes in 0.27-maintenance into master and publish 0.28 later this year. I'll make the release and do the publishing on exiv2.org.

How does that sound to you?

…hough this is working on 0.27-maintenance)

lgtm-com · 2020-05-02T16:54:22Z

This pull request introduces 3 alerts when merging f1182b8 into 6e5fdb0 - view on LGTM.com

new alerts:

2 for Comparison result is always the same
1 for Multiplication result converted to larger type

piponazo · 2020-05-02T17:59:30Z

README.md

+
+### 2.17 Building with C++11 and other compilers
+
+Exiv2 uses the default compiler for your system.  Exiv2 v0.27 was written to the C++ 1998 standard and will compile with C++11 and C++14.  As auto_ptr support is no longer in C++17, you cannot build with that system.  Exiv2 v0.28 and later do not use auto_ptr and are fully supported with all compilers.


I would replace here the last part of the sentence: "supported with all compilers".
We should say that we require a C++ compiler with support for the C++11 standard.

piponazo · 2020-05-02T18:00:00Z

README.md

+```bash
+$ cd <exiv2dir>
+$ mkdir build ; cd build
+$ cmake .. -DCMAKE_CXX_STANDARD=11 -DCMAKE_CXX_FLAGS=-Wno-deprecated


Great! This is exactly how it should be used. I did not think about it before.

piponazo · 2020-05-02T18:05:30Z

Ey @clanmills this PR broke the CI in most of the services, let me know if I can help you with this.

clanmills · 2020-05-02T18:11:23Z

The CI is broken because the output log is so long with all the deprecation warnings. We should probably update the CI to use -Wno_deprecated

I removed the geotag related stuff from the PR, and it removed the files from the 0.27-maintenance branch.

Let me get the product back together and I'll build it on the Mac Mini (macOS, Ubuntu, Cygwin, MinGW, VS2019, SunOS, FreeBSD and NetBSD). Then I'll relax.

clanmills added 4 commits May 1, 2020 16:18

fix_1046_sample_geotag Take advantage of fix to #1046 in samples/geot…

759ce9f

…ag.cpp

Added option -remove to samples/geotag.cpp

d994161

Fixing reference output from test/geotag-test.sh

4da0a57

Adding option -DEXIV2_BUILD_USE_C++11

63a13e8

clanmills added this to the v0.27.3 milestone May 2, 2020

clanmills requested a review from piponazo May 2, 2020 11:35

clanmills self-assigned this May 2, 2020

clanmills added 2 commits May 2, 2020 12:47

Remove target_compile_features(exiv2 PRIVATE cxx_std_98) from src/exiv2

cd5c3e4

Always include <memory.h> before discussing auto_ptr

e0f65f7

piponazo requested changes May 2, 2020

View reviewed changes

clanmills added 4 commits May 2, 2020 14:22

Updated documentation. Removed -DEXIV2_BUILD_USE_C++11

65dde52

Change config.h to compile isatty() and getpid() on macOS when -DCMAK…

c423b91

…E_CXX_STANDARD=98

Fix auto_ptr to auto\_ptr

3ebfee1

Remove add_compile_options(-std=gnu++98)

b22d1de

clanmills added 5 commits May 2, 2020 16:48

Documentation polishing.

7928c63

Issue identified in #1101 master/MinGW/Qt. Apply the change here (alt…

0a06dc9

…hough this is working on 0.27-maintenance)

Print summary to report compiler and c++ standard.

4060298

Don't report CMAKE_CXX_COMPILER - it's on the previous line of output.

da91f04

Only #include<memory> when C++ >= C++11

f1182b8

clanmills added 4 commits May 2, 2020 18:29

Fixing a typo in the documentation.

9b3f47f

Delete geotag-test.out

f775676

Delete geotag.cpp

cd9ee99

Delete README-SAMPLES.md

9b6b403

clanmills merged commit 93cad8c into 0.27-maintenance May 2, 2020

clanmills deleted the fix_c++11_0.27 branch May 2, 2020 17:42

piponazo reviewed May 2, 2020

View reviewed changes

clanmills mentioned this pull request May 19, 2020

Exiv2 RoadMap #1018

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix c++11 0.27 #1194

Fix c++11 0.27 #1194

clanmills commented May 2, 2020

piponazo May 2, 2020

piponazo May 2, 2020

clanmills May 2, 2020

piponazo May 2, 2020

clanmills May 2, 2020

piponazo May 2, 2020

clanmills May 2, 2020

clanmills commented May 2, 2020 •

edited

Loading

clanmills commented May 2, 2020

lgtm-com bot commented May 2, 2020

piponazo May 2, 2020

piponazo May 2, 2020

piponazo commented May 2, 2020

clanmills commented May 2, 2020

		@@ -1,5 +1,9 @@
		# These flags applies to exiv2lib, the applications, and to the xmp code

		if ( EXIV2_BUILD_USE_C++11 )


		### 2.17 Building with C++11 and other compilers

		Exiv2 uses the default compiler for your system. Exiv2 v0.27 was written to the C++ 1998 standard and will compile with C++11 and C++14. As auto_ptr support is no longer in C++17, you cannot build with that system. Exiv2 v0.28 and later do not use auto_ptr and are fully supported with all compilers.

Fix c++11 0.27 #1194

Fix c++11 0.27 #1194

Conversation

clanmills commented May 2, 2020

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

clanmills commented May 2, 2020 • edited Loading

2.17 Building with C++11 and other compilers

clanmills commented May 2, 2020

lgtm-com bot commented May 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piponazo commented May 2, 2020

clanmills commented May 2, 2020

clanmills commented May 2, 2020 •

edited

Loading