Opened 5 years ago

Closed 3 years ago

#3845 closed task (duplicate)

Compile Kea with the C++11 enabled

Reported by: marcin Owned by: UnAssigned
Priority: medium Milestone: Outstanding Tasks
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 (last modified by fdupont)

The following tasks have to be conducted to make sure that Kea is compilable with the C++11 features ON:

  • Enable C++11 compilation and see it fail (presumably on auto_ptr's)
  • Fix the issues with the use of auto_ptr's with C++11
  • Fix any other issues that may occur when compiling with C++11
  • Make sure Kea still successfully compiles after fixes applied, when using the C++98
  • Make sure that QA enables C++11 compilation switch on the build farm on some systems

Subtickets

Change History (26)

comment:1 Changed 5 years ago by fdupont

  • Description modified (diff)
  • Owner set to fdupont
  • Status changed from new to accepted
  • Summary changed from Compile Kea with the C++11 enbled to Compile Kea with the C++11 enabled

comment:2 Changed 5 years ago by fdupont

For instance the std::auto_ptr's in src/lib/util/tests/memory_segment_local_unittest.cc could be easily replaced by boost::scoped_ptr's.

comment:3 Changed 5 years ago by fdupont

Botan 1.11 is not compatible (yet): botan-config (used by ./configure) was dropped in favour of 'botan config'. I wrote a botan-script to replace it but the API changed a lot too so even ./configure failed its sanity tests...

BTW the quick & dirty script:

#!/bin/sh

export DYLD_LIBRARY_PATH=/Users/dupont/dev/c11/botan/lib

case $1 in
  --prefix) a=prefix ;;
  --cflags) a=cflags ;;
  --ldflags) a=ldflags ;;
  --libs) a=libs ;;
  *) a= ;;
esac

exec /Users/dupont/dev/c11/botan/bin/botan config $a

comment:4 Changed 5 years ago by fdupont

Summary:

  • no auto_ptr warnings or errors?
  • bind(2) vs bind template -> ::bind
  • EXPECT_TRUE(<boost smart pointer>) -> EXPECT_TRUE(ptr.get() != 0) , i.e., using the official conversion to a bool type. BTW EXPECT_FALSE() works well...
  • << with an ostringstream at the right side -> add .str() which extracts the string content.

Not finished...

comment:5 Changed 5 years ago by fdupont

Done for OS X, exactly:

  • g++ c++11
  • openssl, gtest (1.7.0 sources), memfile (i.e., no database) g++ --version returns:
    Apple LLVM version 6.1.0 (clang-602.0.49) (based on LLVM 3.6.0svn)
    Target: x86_64-apple-darwin14.3.0
    Thread model: posix
    

Going on the last Ubuntu on a 32 bit VM...

comment:6 Changed 5 years ago by fdupont

Ubuntu 15.04 compiler: g++ (Ubuntu 4.9.2-10ubuntu13) 4.9.2

comment:7 Changed 5 years ago by fdupont

BTW I git the XXX_TRUE(<smart_pointer>) error on pkt_filter_lpf_unittest.cc so g++ 4.9.2 also wants an explicit test...

comment:8 Changed 5 years ago by fdupont

Note I got 2 auto_ptr is deprecated warnings on logger_manager_impl.cc (not fixable as the auto_ptr's are in the log4cplus API) which is (as the whole log library?) compiled without -Werror.

comment:9 follow-up: Changed 5 years ago by marcin

I saw you adding this:

ASSERT_TRUE(some_pointer.get() != 0)

Couldn't you just do it like this:

ASSERT_TRUE(some_pointer != true)

or better

ASSERT_FALSE(some_pointer == true) ?

comment:10 Changed 5 years ago by fdupont

Finished on Ubuntu with a recent (IMHO equivalent of stable) version of the GNU C++ compiler.
Ready for review.

(note: we should try with the database backends too but we have time. IMHO c++11 should be added to the Jenkins configs...).

comment:11 Changed 5 years ago by fdupont

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

comment:12 in reply to: ↑ 9 Changed 5 years ago by fdupont

Replying to marcin:

I saw you adding this:

ASSERT_TRUE(some_pointer.get() != 0)

Couldn't you just do it like this:

ASSERT_TRUE(some_pointer != true)

or better

ASSERT_FALSE(some_pointer == true) ?

Note I believe you mean ASSERT_TRUE(some_pointer == true) ...

I imagined once to just use {{ !! <smart pointer> }} but:

  • the problem is the compiler can't find a conversion from the smart pointer to a bool
  • the boost documentation says:
    • the operator is explicit operator bool() const; // never throws
    • its semantics (i.e., what it Returns:) is get() != 0 so I simply expanded the semantics...

Anyway there is no solution which is pleasant to read...
BTW I know why the ASSERT_FALSE() doesn't give the same issue: ASSERT/EXPECT_TRUE/FALSE are (old CPP) macros with a !(condition) for the FALSE side calling GTEST_TEST_BOOLEAN_()...

To finish if you really prefer another style than the .get() != 0 you can substitute it directly in a diff and apply it. The initial tag is trac3845_base, the current state of the branch has no tag (yet).

comment:13 Changed 5 years ago by fdupont

The source of the ASSERT_TRUE() problem is the explicit conversion operator which is new in C++11 (cf the wikipedia). So now we fully understand the problem (which is IMHO a google test bug, as the author of this path (using !! ) believes too (:-)) https://groups.google.com/forum/#!topic/googletestframework/uGgC5dopP50

comment:14 Changed 5 years ago by fdupont

Decision: include after test/gtest.h a test-fixup.h with a fixed definition of ASSERT_TRUE and EXPECT_TRUE macros.
A bit related to #3827 too.

comment:15 Changed 5 years ago by fdupont

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

comment:16 Changed 5 years ago by fdupont

Done with the gtest-fixup.h include. Add detection of the explicit conversion operator support in configure.ac.
Proposed rules are:

  • no config.h in includes
  • avoid gtest.h in includes because of the previous rule
  • config.h first in all .cc files which include gtest.h (directly form the previous rule)
  • all files including gtest.h must include gtest-fixup.h just after
  • if an include file uses gtest macros, it must be included after gtest.h (and gtest-fixup.h in application of the previous rule) and a comment referring to this rule must be added in the include file.

These rules were partially applied so the work is not finished...

comment:17 Changed 5 years ago by fdupont

Summary of issues introduced by -std=c++11:

  • ostringstream's can no longer go at right hand of operator {{ << }}, for instance:
    oss_ << "foo" << stream;
    

where oss_ and stream are ostringstream objects.
The solution is to get the string, i.e.:

oss_ << "foo" << stream.str();
  • explicit conversion operator on to smart pointers to bool. A smart pointer can be converted into a boolean only in a context which requires a boolean, for instance:
    #include <boost/shared_ptr.hpp>
    
    boost::shared_ptr<int> foo();
    
    bool bar() { return(foo()); }
    

raises a no viable conversion from 'boost::shared_ptr<int>' to 'bool' error.
This is fixed for gtest ASSERT_TRUE() and EXPECT_TRUE() but there are (were!) a few other occurrences in the code. The solution is to force a boolean context by !! or (IMHO better) to explicit the non-null property by .get() != 0 .

comment:18 Changed 5 years ago by fdupont

Merging #3848 (unused includes).

comment:19 Changed 5 years ago by fdupont

Removed the unused resolver.h include too.
Works well on my OS X with and without -std=c++11 (can check Ubuntu too but the VM is slowww).
Ready for review but:

  • no idea for a ChangeLog yet
  • the rules about gtest and co should go somewhere in the doc (where? I can propose a better written text).

comment:20 Changed 5 years ago by fdupont

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

comment:21 Changed 5 years ago by fdupont

As this ticket implies a lot of small changes in the beginning of a lot of files it should be reviewed ASAP or it could become hard to merge or force to rebase some other branches/tickets before merging...

comment:22 follow-up: Changed 5 years ago by tomek

  • Milestone changed from Kea-proposed to DHCP Outstanding Tasks

comment:23 in reply to: ↑ 22 Changed 4 years ago by fdupont

Replying to tomek:

=> I understand why you want to postpone this but IMHO it is not the best thing to do. I propose to split this ticket into:

  • move the removal of unused files to #4008
  • check if 1.6.0 and git versions of gtest don't show the XXX_TRUE() bug. If it is the case (as I expect) update the documentation
  • create a new ticket for the few other changes. As they includes isc_throw() macros this should be done after #3856 in Kea1.0.

The reason is we shall likely find compilers which limit conversion as required in C++11 even they are not officially C++11 "only" compilers (this was introduced in C++11 to avoid some bugs so it can be considered as an improvement which should be back ported and BTW applying this makes the code clearer/cleaner too).

comment:24 Changed 4 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

comment:25 Changed 3 years ago by fdupont

  • Description modified (diff)

Mostly replaced by #4631.

comment:26 Changed 3 years ago by fdupont

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

Closing it as a duplicate.

Note: See TracTickets for help on using tickets.