Opened 3 years ago

Closed 3 years ago

#4631 closed task (complete)

c++11 second round

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea1.2
Component: build system Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Previous c++11 stuff was divided into short term (and merged) stuff and long term (including gtest) stuff (aka #3845). This ticket proposes to get the C++11 changes at the exclusion of the gtest changes (but keeping the check in configure) and making #3845 a duplicate which can be closed.
Note this ticket is for after 1.1.

Subtickets

Change History (25)

comment:1 Changed 3 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:2 Changed 3 years ago by fdupont

Done. I'll put it under review when the milestone will be decided.

comment:3 Changed 3 years ago by stephen

  • Milestone changed from Kea-proposed to Kea1.2

comment:4 Changed 3 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

comment:5 Changed 3 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 follow-up: Changed 3 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit 5518153dbade98397c29a507a4e9617e6f27c89f

configure.ac
This may be something that was done in trac3845, but there appears to be nothing in configure.ac that checks whether the compiler is C++ enabled. Indeed, there doesn't seem to be anything in configure.ac that enables C++11 mode in the compiler.

In configure.ac, if I change the newly added test:

       [AC_LANG_PROGRAM(
           [#include <boost/shared_ptr.hpp>
	    #include <gtest/gtest.h>
	    void foo() {
	        boost::shared_ptr<int> bar;
		ASSERT_TRUE(bar);
            }],
	    [return 0;])],

(which does not use a C++11 construct) to

       [AC_LANG_PROGRAM(
           [#include <memory>
	    #include <gtest/gtest.h>
	    void foo() {
                std::unique_ptr<int> bar;
		ASSERT_TRUE(static_cast<bool>(bar));
            }],
	    [return 0;])],
	

(which does), then on my Unbuntu 12.04 machine, the test fails.

However, if I run configure with the command:

CXXFLAGS=-std=c++0x ./configure

... it passes.

Notes:

  1. The compiler I'm using on Ubuntu 12.04 is gcc 4.6.3. This is quite old, and it does not recognise "-std=c++11". However, according to the GNU documentation, "c++0x" is a deprecated synonym for "c++11".
  1. This may be due to the version of gtest I am using (1.7.0) but the documentation for std::unique_ptr says that it has "operator bool" defined. However, without the static_cast statement, the test compilation fails.

Summary: I think that configure.ac should test whether the compiler can compile a C++11 program, testing it first without any flags, then with "-std=c++11" then with "-std-c++0x".

src/lib/util/tests/memory_segment_local_unittest.cc
Why replace the auto_ptr with a boost::scoped_ptr here when std::unique_ptr has been used everywhere else? (As an aside, my preference is to use STL first then Boost: I've always thought of some of the Boost constructs (such as scoped_ptr) as temporary place holders until the functionality was available in STL.)

ChangeLog
I think a ChangeLog entry is required. Perhaps something like:

Modifications to replace auto_ptr with the C++11 unique_ptr.

Other
When building, I get a few warnings about use of auto_ptr in files in the the src/lib/log directory.

I also get the following error:

  CXX    libkea_dhcp___la-iface_mgr_linux.lo
iface_mgr_linux.cc: In member function 'void {anonymous}::Netlink::rtnl_open_socket()':
iface_mgr_linux.cc:146:63: error: no match for 'operator<' in 'std::bind(_Functor&&,
_ArgTypes&& ...) [with _Functor = int&, _ArgTypes = {sockaddr*, unsigned int}, typename
std::_Bind_helper<_Functor, _ArgTypes>::type = std::_Bind<int(sockaddr*, unsigned int)>]((* &
isc::util::io::internal::convertSockAddr [with SAType = sockaddr_nl]
((&(({anonymous}::Netlink*)this)->{anonymous}::Netlink::local_))), (* &12u)) < 0'

I think this occurrence of "bind" should be replaced with "::bind", as done in src/lib/dhcpsrv/dhcp4o6_ipc.cc.

comment:7 in reply to: ↑ 6 Changed 3 years ago by fdupont

  • Owner changed from fdupont to stephen

Replying to stephen:

Reviewed commit 5518153dbade98397c29a507a4e9617e6f27c89f

configure.ac
This may be something that was done in trac3845, but there appears to be nothing in configure.ac that checks whether the compiler is C++ enabled.

=> I disagree: this is done by AC_PROG_CXX

Indeed, there doesn't seem to be anything in configure.ac that enables C++11 mode in the compiler.

=> I can't see a good reason to have this kind of thing in configure.ac.

In configure.ac, if I change the newly added test:

       [AC_LANG_PROGRAM(
           [#include <boost/shared_ptr.hpp>
	    #include <gtest/gtest.h>
	    void foo() {
	        boost::shared_ptr<int> bar;
		ASSERT_TRUE(bar);
            }],
	    [return 0;])],

(which does not use a C++11 construct) to

       [AC_LANG_PROGRAM(
           [#include <memory>
	    #include <gtest/gtest.h>
	    void foo() {
                std::unique_ptr<int> bar;
		ASSERT_TRUE(static_cast<bool>(bar));
            }],
	    [return 0;])],
	

(which does), then on my Unbuntu 12.04 machine, the test fails.

=> perhaps this means only your Ubuntu 12.04 is not c++11 by default.

However, if I run configure with the command:

CXXFLAGS=-std=c++0x ./configure

... it passes.

=> I don't know if there is a very easy way (i.e., command line) to know what is the default C++ version of a compiler. Note there is a not very hard way using some code and the cplusplus macro. If you suggest to add something printing it in configure.ac IMHO it is not a bad idea (BTW this should be done by the AC_PROG_CXX macro but currently is not).

Notes:

  1. The compiler I'm using on Ubuntu 12.04 is gcc 4.6.3. This is quite old, and it does not recognise "-std=c++11". However, according to the GNU documentation, "c++0x" is a deprecated synonym for "c++11".

=> IMHO it should be a good idea to switch to a more recent Ubuntu for C++11 stuff.

  1. This may be due to the version of gtest I am using (1.7.0) but the documentation for std::unique_ptr says that it has "operator bool" defined. However, without the static_cast statement, the test compilation fails.

=> there is a known bug in test 1.7.0 which makes it to fail to compile with C++11.

Summary: I think that configure.ac should test whether the compiler can compile a C++11 program, testing it first without any flags, then with "-std=c++11" then with "-std-c++0x".

=> I disagree: this is not the job of configure.

src/lib/util/tests/memory_segment_local_unittest.cc
Why replace the auto_ptr with a boost::scoped_ptr here when std::unique_ptr has been used everywhere else? (As an aside, my preference is to use STL first then Boost: I've always thought of some of the Boost constructs (such as scoped_ptr) as temporary place holders until the functionality was available in STL.)

=> std::unique_ptr does not compile without C++11 on old compilers (e.g., your compiler). And scope_ptr has a minimal footprint so anyway for this code is a good (i.e., perhaps better than auto/unique_ptr) candidate.

ChangeLog
I think a ChangeLog entry is required. Perhaps something like:

Modifications to replace auto_ptr with the C++11 unique_ptr.

=> NO! The purpose of this ticket is to make current code C++11 friendly, not to break C++<11 compilers.

Other
When building, I get a few warnings about use of auto_ptr in files in the the src/lib/log directory.

=> yes and as they are in the log4cplus APIs they stay. Note there is a companion ticket (#4636) for the log4cplus 2.x where these auto_ptr where replaced.

I also get the following error:

  CXX    libkea_dhcp___la-iface_mgr_linux.lo
iface_mgr_linux.cc: In member function 'void {anonymous}::Netlink::rtnl_open_socket()':
iface_mgr_linux.cc:146:63: error: no match for 'operator<' in 'std::bind(_Functor&&,
_ArgTypes&& ...) [with _Functor = int&, _ArgTypes = {sockaddr*, unsigned int}, typename
std::_Bind_helper<_Functor, _ArgTypes>::type = std::_Bind<int(sockaddr*, unsigned int)>]((* &
isc::util::io::internal::convertSockAddr [with SAType = sockaddr_nl]
((&(({anonymous}::Netlink*)this)->{anonymous}::Netlink::local_))), (* &12u)) < 0'

I think this occurrence of "bind" should be replaced with "::bind", as done in src/lib/dhcpsrv/dhcp4o6_ipc.cc.

=> yes, it is the standard fix but it is highly compiler dependent so easy to miss. Fixed.

comment:8 follow-up: Changed 3 years ago by stephen

  • Owner changed from stephen to fdupont

This may be something that was done in trac3845, but there appears to be nothing in configure.ac that checks whether the compiler is C++ enabled.

I disagree: this is done by AC_PROG_CXX

That was my mistake - I meant to say C++ 11 enabled.

Indeed, there doesn't seem to be anything in configure.ac that enables C++11 mode in the compiler.

I can't see a good reason to have this kind of thing in configure.ac.

The problem is that not all compilers compile C++ 11 by default. Although my original tests were done on Ubuntu 12.04 with g++ 4.6.3, I have just installed Ubuntu 16.04 with g++ 5.4.0 on a virtual machine. Compiling:

#include <memory>
int main(int argc, char**) {
   std::unique_ptr<int> a;
   return 0;
}

with the raw "g++" command fails - it doesn't recognise unique_ptr. With the "-std=c++11" switch, the compilation succeeds.

I can't see a good reason to have this kind of thing in configure.ac.
:

Summary: I think that configure.ac should test whether the compiler can compile a C++11 program, testing it first without any flags, then with "-std=c++11" then with "-std-c++0x".

I disagree: this is not the job of configure.

It is the job of configure. Configure is supposed to determine the environment in which the program is being built and set up flags accordingly. Without setting the appropriate C++ 11 switch, users on some systems are not going to be able to build Kea without explicitly setting CXXFLAGS before running configure.

The following code should, if added to configure.ac at the appropriate point, set the correct C++ 11 flag for the compilation.

AC_MSG_CHECKING([compiler for C++ 11 support])
SAVE_CXXFLAGS="$CXXFLAGS"
AC_COMPILE_IFELSE(
    [AC_LANG_PROGRAM(
        [#include <memory>],
        [std::unique_ptr<int> a;]
    )],
    [AC_MSG_RESULT([yes])]
    [
        CXXFLAGS="$SAVE_CXXFLAGS -std=c++11"
        AC_COMPILE_IFELSE(
            [AC_LANG_PROGRAM(
                [#include <memory>],
                [std::unique_ptr<int> a;]
            )],
            [AC_MSG_RESULT([yes])]
            [
                CXXFLAGS="$SAVE_CXXFLAGS -std=c++0x"
                AC_COMPILE_IFELSE(
                    [AC_LANG_PROGRAM(
                        [#include <memory>],
                        [std::unique_ptr<int> a;]
                    )],
                    [AC_MSG_RESULT([yes])]
                    [AC_MSG_ERROR([compiler does not support C++ 11])]
                )
            ]
        )
    ]
)

src/lib/util/tests/memory_segment_local_unittest.cc
Why replace the auto_ptr with a boost::scoped_ptr here when std::unique_ptr has been used everywhere else? (As an aside, my preference is to use STL first then Boost: I've always thought of some of the Boost constructs (such as scoped_ptr) as temporary place holders until the functionality was available in STL.)

std::unique_ptr does not compile without C++11 on old compilers (e.g., your compiler). And scope_ptr has a minimal footprint so anyway for this code is a good (i.e., perhaps better than auto/unique_ptr) candidate.

The point I was trying to make was about consistency. Let's use either boost::scoped_ptr or std::unique_ptr in Kea, but not both.

ChangeLog
Accept you point about C++11 consistency. How about a single entry for all C++11-related tickets, say:

Changes to allow Kea to be more compatible with C++ 11

comment:9 in reply to: ↑ 8 Changed 3 years ago by fdupont

Replying to stephen:

This may be something that was done in trac3845, but there appears to be nothing in configure.ac that checks whether the compiler is C++ enabled.

I disagree: this is done by AC_PROG_CXX

That was my mistake - I meant to say C++ 11 enabled.

=> why? This ticket is about C++11 compatibility, *not* about requiring C++11.

Indeed, there doesn't seem to be anything in configure.ac that enables C++11 mode in the compiler.

I can't see a good reason to have this kind of thing in configure.ac.

The problem is that not all compilers compile C++ 11 by default.

=> yes but IMHO the answer should not be to enforce C++>=11 mode.

With the "-std=c++11" switch, the compilation succeeds.

=> but this is not required to compile Kea. BTW if you really believe we should require C++11 then another ticket should be opened and we'll have to fix Botan and log4cplus versions too (so it won't be an easy change).

It is the job of configure. Configure is supposed to determine the environment in which the program is being built and set up flags accordingly. Without setting the appropriate C++ 11 switch, users on some systems are not going to be able to build Kea without explicitly setting CXXFLAGS before running configure.

=> the question is about requiring or not C++11.

The point I was trying to make was about consistency. Let's use either boost::scoped_ptr or std::unique_ptr in Kea, but not both.

=> I don't understand: unique_ptr is a replacement for auto_ptr, scoped_ptr is something different so there is no problem to use both.

ChangeLog
Accept you point about C++11 consistency. How about a single entry for all C++11-related tickets, say:

Changes to allow Kea to be more compatible with C++ 11

=> even I agree with this ChangeLog it is clearly about compatibility, not requirement...
BTW I created a ticket for the display of the C++ version.

comment:10 Changed 3 years ago by fdupont

  • Owner changed from fdupont to stephen

comment:11 Changed 3 years ago by stephen

  • Owner changed from stephen to fdupont

but this is not required to compile Kea. BTW if you really believe we should require C++11 then another ticket should be opened and we'll have to fix Botan and log4cplus versions too (so it won't be an easy change

OK, I see the confusion. I have interpreted this ticket to mean change Kea to use the C++11 compiler. You have interpreted it as "allow Kea to be compiled with a C++11 compiler without generating warnings". At the moment though, the changes so far achieve neither of these goals.

I mentioned above that g++ 5.4.0 on Ubuntu 16.04 does not enable C++11 mode by default. If I try to compile the latest commit on this branch (331103fe37d93a5c88fb7b834e64e0b3d64530e5) using that combination, the compilation fails because the compiler does not recognise std::unique_ptr. I can get the branch to compile if I define CXXFLAGS to be "-std=c++11" before invoking configure, but I don't think this is desirable behaviour.

Changes in this ticket replaced the original auto_ptr with unique_ptr in the following files:

src/lib/dns/master_loader.cc
src/lib/dns/rdata.cc
src/lib/dns/rdataclass.cc
src/lib/dns/rdata/any_255/tsig_250.cc
src/lib/dns/rdata/generic/dnskey_48.cc
src/lib/dns/rdata/generic/nsec3_50.cc
src/lib/dns/rdata/generic/nsec3param_51.cc
src/lib/dns/rdata/generic/opt_41.cc
src/lib/dns/rdata/generic/rrsig_48.cc
src/lib/dns/rdata/generic/sshfp_44.cc
src/lib/util/threads/sync.cc
src/lib/util/threads/thread.cc

Looking at the code, I see that we can't replace the unique_ptr with a scoped_ptr in these files because of the requirement to transfer ownership of the pointed-to object, a facility that scoped_ptr does not possess.

Unless we just don't merge this branch, I can't see any alternative to enabling C++11 compilation by getting configure to generate appropriate flags for the C++ command: that is the change to configure.ac that is listed above.

As to the packages:

  • I'm not sure about Botan or OpenSSL.
  • Version of log4cplus before 1.2 will compile, although a warning will be output because one of the functions returns an auto_ptr.
  • We will need the latest Google Test (1.8) - although that is only used if you want to run the unit tests.

However, requiring certain versions of packages with a new version of Kea is not a show-stopper.

comment:12 Changed 3 years ago by fdupont

I see: this is an old ticket so it is not in the state I believed. I think we have to add some ifdefs...
About the package version I agree but there is nothing we can really do about Google Test other than pushing the from source option (which according to the Google Test documentation should be the only allowed one).

comment:13 Changed 3 years ago by fdupont

I created 2 child tickets (#5050 and #5051).

comment:14 Changed 3 years ago by fdupont

  • Owner changed from fdupont to stephen

Ready for review. Note #5049 and #5051 are parts of this framework and should be merged before.

comment:15 follow-up: Changed 3 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit 275bdf1e39e1d3020376e4a64b2fdee92d326cfd.

The change of "bind" to "::bind" in src/lib/dhcp/iface_mgr_linux.cc is fine.

However, as noted above, this ticket cannot be merged as-is because the branch fails to build under Linux 16.04.

You raised three question in the Kea meeting on 2011-11-03:

1. Enforce C++11 or not
The answer to this is "yes". We want to be able to use C++11 constructs in Kea code.

2. When to enforce C++11
I'm not sure what you mean by this, but I would say that you "enforce" C++11 if the native compiler without any switches is unable to compile a C++11 test program. Then "configure" needs to work out what switches to include in the command line. (The alternative - telling the user to set CXX to the combination of compiler command and switches - immediately begs the question: "why can't 'configure' do that?")

3. How to enforce C++11
In the configure.ac snippet I gave in a previous comment, "configure" first tries to compiler a C++11 program without any flags. If that succeeds, no flags are added to the compiler command. It then tries "-std=c++11", adding that flag to the compile command if the compiler accepts it and the test program compiles successfully. Finally it tries "-std=c++0x": that is only there for very old compilers (as found in long-term support versions of the operating system). We could try additional switches, but the basic principle remains unchanged - try a number of switches until one is found that allows the test program to compile.

The test program in the example I gave checks C++11 compliance by creating a std::unique_ptr. Arguably, it should use a few more C++ constructs. However, other checks need to be made. We know that gtest 1.8.0 or later is required, so if compiling with gtest, configure should check the version. As I mention above, I'm not sure if there are restrictions on what version of OpenSSL or Botan may be used. If there are, they should be checked.

In Summary
This ticket (or another ticket that is merged at the same time) should

  1. modify configure.ac to allow it to work out what switches are needed to compile C++11 code.
  2. modify configure.ac to check for the minimum versions of packages (such as googletest) required for compilation.

comment:16 in reply to: ↑ 15 Changed 3 years ago by fdupont

Replying to stephen:

Reviewed commit 275bdf1e39e1d3020376e4a64b2fdee92d326cfd.

The change of "bind" to "::bind" in src/lib/dhcp/iface_mgr_linux.cc is fine.

However, as noted above, this ticket cannot be merged as-is because the branch fails to build under Linux 16.04.

=> I'll find what is the problem and fix it.

You raised three question in the Kea meeting on 2011-11-03:

1. Enforce C++11 or not
The answer to this is "yes". We want to be able to use C++11 constructs in Kea code.

2. When to enforce C++11
I'm not sure what you mean by this

=> at what time: now, tomorrow, when we'll add a C++11 depency, etc. Note #5050 should be part of the answer so I propose to move this point to it (i.e., warning vs error).

3. How to enforce C++11

=> we clearly disagree about what to put in configure. Personally I strongly dislike a package which blindly changes the compiler setting in such a way...

The test program in the example I gave checks C++11 compliance by creating a std::unique_ptr. Arguably, it should use a few more C++ constructs.

=> any C++11 specific construct should be tested at least because some compilers have partial C++11 compliance.

However, other checks need to be made. We know that gtest 1.8.0 or later is required, so if compiling with gtest, configure should check the version.

=> it is not so clear: very old gtest versions don't work, old versions works, not very recent versions don't work, 1.8.0 and development versions work (and fail to return a version in configure...). So the current way (check the problematic macro) is IMHO the right one.

As I mention above, I'm not sure if there are restrictions on what version of OpenSSL or Botan may be used.

=> OpenSSL is in pure C so is a fully independent problem. Botan <= 1.10 is C++98, Botan => 1.11 is C++11. Now I don't know any system where the Botan package is 1.11. Same for log4cplus...

In Summary
This ticket (or another ticket that is merged at the same time) should

=> I agree but not for the "is merged at the same time": there is no urgent need to enforce C++11. In fact the only thing urgent when this (fixed) branch will be merged is to deal with #5050 ASAP (fortunately #5050 is trivial to implement).

  1. modify configure.ac to allow it to work out what switches are needed to compile C++11 code.
  2. modify configure.ac to check for the minimum versions of packages (such as googletest) required for compilation.

=> I've just created #5066 to address this.

I am taking the ticket the time to fix the branch on Ubuntu 16.04

comment:17 Changed 3 years ago by fdupont

  • Owner changed from fdupont to stephen

I am building trac4631a on Ubuntu 16.04 with and without C++11: it no longer fails.

comment:18 follow-ups: Changed 3 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit 455d99e15388b6f107a5b060f2b54bff6ce55dd6

I'm sorry, but I don't think this is the right solution.

It adds a number of #ifdefs that will ultimately have to be removed when we enforce C++11. Although they avoid warnings about deprecated features for most of the code, owing to use of log4cplus 1.x, the code in the src/lib/log directory uses auto_ptr so the compilation will still generate a deprecated feature warning.

I suggest the following strategy:

For this ticket:

  1. Make the changes that do not involve unique_ptr (i.e. replacing "bind" by "::bind" etc.)
  2. Add the -Wno-deprecated flag to the compilation flags. Note: this is a temporary solution and one that will be backed out when we move fully to C++.

We can then commit the changes and know that Kea will build in both C++11 and non-C++11 modes without warnings.

In ticket 5066:

  1. Remove -Wno-deprecated from the compiler flags
  2. Add the code to configure.ac needed to enforce C++11 compilation.
  3. To allow users to continue using log4cplus 1.x, add a check in configure.ac to see what version of log4cplus is being used and set a macro in config.h based on that. Add #ifdefs to the relevant src/lib/log files to allow for either auto_ptr/unique_ptr. (We could also modify src/lib/log/Makefile.am to use -Wno-deprecated if building logging for log4cplus 1.x)
  4. Add a check in configure.ac to check for the version of gtest installed is the user wants to build the tests as well.

comment:19 in reply to: ↑ 18 Changed 3 years ago by fdupont

Replying to stephen:

For this ticket:

  1. Make the changes that do not involve unique_ptr (i.e. replacing "bind" by "::bind" etc.)

=> for the not involve I believe you mean keep auto_ptr.

  1. Add the -Wno-deprecated flag to the compilation flags. Note: this is a temporary solution and one that will be backed out when we move fully to C++.

=> it is very dangerous: the deprecation mechanism is used for many other things and this will hide potential problems. In general I don't like general settings to hide a few local problems (i.e., I consider this as a bad practice).

In ticket 5066:

=> I am cut paste this to 5066.

comment:20 in reply to: ↑ 18 Changed 3 years ago by fdupont

Replying to stephen:

Reviewed commit 455d99e15388b6f107a5b060f2b54bff6ce55dd6

I'm sorry, but I don't think this is the right solution.

It adds a number of #ifdefs that will ultimately have to be removed when we enforce C++11. Although they avoid warnings about deprecated features for most of the code, owing to use of log4cplus 1.x, the code in the src/lib/log directory uses auto_ptr so the compilation will still generate a deprecated feature warning.

=> I'd like to keep them until we move to C++11 because they can be removed using unifdef without pain.

comment:21 Changed 3 years ago by fdupont

Argh, the simple parser requires C++11 and uses another specific feature...
I propose to merge this and #5050 into #5066.

Last edited 3 years ago by fdupont (previous) (diff)

comment:22 Changed 3 years ago by fdupont

I built a trac4631b branch with the not-unique_ptr stuff.
Ready for a quick review and immediate merge.

comment:23 Changed 3 years ago by fdupont

  • Owner changed from fdupont to UnAssigned

comment:24 Changed 3 years ago by stephen

  • Owner changed from UnAssigned to fdupont

Reviewed trac4631b branch (commit 3b8787a7a8d62e28789bb909861bccd957e66e4a)

All OK, please merge.

These changes are trivial and I do not believe they require a ChangeLog entry.

comment:25 Changed 3 years ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged. Closing.

Note: See TracTickets for help on using tickets.