Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#4024 closed defect (fixed)

critical c++11 issues

Reported by: fdupont Owned by: fdupont
Priority: low Milestone: Kea1.0-beta
Component: Unclassified 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

All blocking c++11 issues so:

  • no auto_ptr issue (at worst auto_ptr's give warnings)
  • no gtest 1.7.0 issue (only this version has a bug with c++11).

Depends on isc_throw tiket (#3856) and a (pretty small) part of #3845.

Subtickets

Change History (12)

comment:1 Changed 5 years ago by fdupont

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

I take the ticket to check the assumption about gtest.

comment:2 Changed 4 years ago by fdupont

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

A proposal (trac4024 branch) ready for review. With it and the last gtest (from subversion vs. buggy 1.7.0) it compiles on my OS X with --std=c++11 without errors. BTW the changes should have no effect with other C++ versions.

comment:3 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.0
  • Priority changed from medium to low

comment:4 Changed 4 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit e2129dd72dd6938f6311f139af49506fd1c736de. The changes look OK. However...

I noted what you said about gtest 1.7.0 being buggy; rather than download a previous gtest from subversion, I downloaded gtest 1.6.0. I built on the Mac using the following combinations:

  • No "-std" switch, gtest 1.6.0: the build failed with the compiler unable to find "tr1/tuple"
  • -std=c++11, gtest 1.6.0: the build failed with the compiler unable to find "tr1/tuple"
  • No "-std" switch, gtest 1.7.0: Kea built OK
  • -std=c++11, gtest 1.7.0: the build failed in src/lib/dns/rrclass_unittest.cc, stating that there was no known conversion from "scoped_ptr<isc::dns::RRClass>" to "const testing::AssertionResult".

The only way that I could get the -std=c++11 to compile with gtest 1.6.0 was to include the following compilation flag: -DGTEST_USE_OWN_TR1_TUPLE. That fixed the compilation of gtest, but then I started running into more "no known conversion" errors.

A number of the changes you made were to change lines of the form:

ASSERT_TRUE(smart_pointer);

to

ASSERT_TRUE(smart_pointer.get() != 0);

(Incidentally, do you know why the {ASSERT,EXPECT}_TRUE form fails, but the {ASSERT,EXPECT}_FALSE one succeeds?)

However, I'm getting these errors when compiling a number of unit tests, many more than the ones you changed. Files that contain them include:

  • lib/cc/tests/command_interpreter_unittests.cc
  • lib/cc/tests/data_unittests.cc
  • lib/dhcp/tests/iface_mgr_unittest.cc
  • lib/dns/tests/message_unittest.cc
  • lib/dns/tests/rrclass_unittest.cc
  • lib/dns/tests/rrttl_unittest.cc

Did you push all the files you edited?

comment:6 in reply to: ↑ 5 Changed 4 years ago by fdupont

  • Owner changed from fdupont to stephen

Replying to stephen:

Reviewed commit e2129dd72dd6938f6311f139af49506fd1c736de. The changes look OK. However...

=> note the #4014 is explicitly about C++ 11 outside gtest related issue (or with other words any issue which is not with a fixed gtest 1.7.0 or the last gtest source from subversion is irrelevant).

I noted what you said about gtest 1.7.0 being buggy; rather than download a previous gtest from subversion, I downloaded gtest 1.6.0. I built on the Mac using the following combinations:

  • No "-std" switch, gtest 1.6.0: the build failed with the compiler unable to find "tr1/tuple"

=> this just means gtest 1.6.0 is too old for the last version of clang++. Not really a surprise.

  • -std=c++11, gtest 1.6.0: the build failed with the compiler unable to find "tr1/tuple"
  • No "-std" switch, gtest 1.7.0: Kea built OK
  • -std=c++11, gtest 1.7.0: the build failed in src/lib/dns/rrclass_unittest.cc, stating that there was no known conversion from "scoped_ptr<isc::dns::RRClass>" to "const testing::AssertionResult".

=> yes, this is an instance of the conversion operator which has new constraints in C++ 11 and is used by gtest 1.7.0 in an incorrect way (bug which doesn't exist in 1.6.0 and was fixed in subversion).

The only way that I could get the -std=c++11 to compile with gtest 1.6.0 was to include the following compilation flag: -DGTEST_USE_OWN_TR1_TUPLE. That fixed the compilation of gtest, but then I started running into more "no known conversion"

errors.

=> so we should not recommend to use gtest 1.6.0...

A number of the changes you made were to change lines of the form:

ASSERT_TRUE(smart_pointer);

to

ASSERT_TRUE(smart_pointer.get() != 0);

=> the first form calls implicitly a conversion operator bool(). It works in C++ < 11 but was fixed in C++ 11.

(Incidentally, do you know why the {ASSERT,EXPECT}_TRUE form fails, but the {ASSERT,EXPECT}_FALSE one succeeds?)

=> there is a ! before (the fix for 1.7.0 is to add a !!).


However, I'm getting these errors when compiling a number of unit tests, many more than the ones you changed. Files that contain them include:

  • lib/cc/tests/command_interpreter_unittests.cc
  • lib/cc/tests/data_unittests.cc
  • lib/dhcp/tests/iface_mgr_unittest.cc
  • lib/dns/tests/message_unittest.cc
  • lib/dns/tests/rrclass_unittest.cc
  • lib/dns/tests/rrttl_unittest.cc

Did you push all the files you edited?

=> yes and there should be no gtest related files (cf. the intent of this ticket vs. #3845).

comment:7 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

yes and there should be no gtest related files (cf. the intent of this ticket vs. #3845).

I'm not clear what you mean here.

I see that #3845 introduces a .h file that contains a redefinition of gtest's XXX_TRUE macros. You seem to be saying that we should go for the XXX_TRUE(smart_pointer.get() != 0) modifications instead of including the gtest macro redefinitions in the code?

If so, as I said above, I am unable to get this branch to build on my OSX system (with either gtest 1.6 or gtest 1.7). The unit tests contain a number of XXX_TRUE(smart_pointer) statements, many more than the ones you changed. I can only get compilation to proceed by making the changes in the various unit tests, so I am surprised that your build succeeds.

comment:8 in reply to: ↑ 7 Changed 4 years ago by fdupont

  • Owner changed from fdupont to stephen

Replying to stephen:

yes and there should be no gtest related files (cf. the intent of this ticket vs. #3845).

I'm not clear what you mean here.

=> #3845 tried to fix everything. #4024 fixes only critical (i.e., blocking) issues and in particular does not try to fix gtest bugs.
So in order of preference one should:

  • use a fixed gtest source, e.g., the current source from subversion, or
  • fix the gtest 1.7.0 (add a !! to the first argument in EXPECT_TRUE() and ASSERT_TRUE definitions in gtest.h, or
  • simply not configure gtest

I see that #3845 introduces a .h file that contains a redefinition of gtest's XXX_TRUE macros. You seem to be saying that we should go for the XXX_TRUE(smart_pointer.get() != 0) modifications instead of including the gtest macro redefinitions in the code?

=> no, even if this solution works this is worse than #3845...

If so, as I said above, I am unable to get this branch to build on my OSX system (with either gtest 1.6 or gtest 1.7).

=> you were not supposed to succeed.

The unit tests contain a number of XXX_TRUE(smart_pointer) statements, many more than the ones you changed.

=> there are none in trac4024...

I can only get compilation to proceed by making the changes in the various unit tests, so I am surprised that your build succeeds.

=> my builds succeeded because I used either current version or fixed 1.7.0 for gtest.

comment:9 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

The unit tests contain a number of XXX_TRUE(smart_pointer) statements, many more than the ones you changed.

=> there are none in trac4024...

? For example, src/lib/dns/tests/rrclass_unittest.cc, lines 104 and 105 are:

    scoped_ptr<RRClass> chclass(RRClass::createFromText("CH"));
    EXPECT_TRUE(chclass);

This failed when I tried to compile with -std=c++11 and passed when I changed the test to

    EXPECT_TRUE(chclass.get() != 0)

There are numerous other examples in the tests.

you were not supposed to succeed

OK, if that was the intension, go ahead and merge. As you said in a separate email, we are not planning on compiling with C++ 11 yet and we have spent a lot of time on this ticket.

comment:10 in reply to: ↑ 9 Changed 4 years ago by fdupont

Replying to stephen:

The unit tests contain a number of XXX_TRUE(smart_pointer) statements, many more than the ones you changed.

=> there are none in trac4024...

=> I was not very clear: I tried to mean there is no XXX_TRUE() fixes in trac4024 (vs there is XXX_TRUE() to be fixed).

? For example, src/lib/dns/tests/rrclass_unittest.cc, lines 104 and 105 are:

    scoped_ptr<RRClass> chclass(RRClass::createFromText("CH"));
    EXPECT_TRUE(chclass);

This failed when I tried to compile with -std=c++11 and passed when I changed the test to

    EXPECT_TRUE(chclass.get() != 0)

There are numerous other examples in the tests.

you were not supposed to succeed

OK, if that was the intension, go ahead and merge. As you said in a separate email, we are not planning on compiling with C++ 11 yet and we have spent a lot of time on this ticket.

=> yes we need first to make some progress (i.e., do vs talking about what to do).

comment:11 Changed 4 years ago by fdupont

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

Merged. Closing.

comment:12 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.