Opened 7 years ago

Closed 7 years ago

#2673 closed defect (fixed)

some build related issues in dhcp

Reported by: jinmei Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130214
Component: dhcp Version:
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

I happened to notice a few issues in the DHCP code. clang++ produced
warnings like this:

../dhcp4_srv.cc:280:1: warning: control may reach end of non-void function [-Wreturn-type]

and it seems to be a real bug. This is the method that triggered the
warning:

bool Dhcpv4Srv::writeServerID(const std::string& file_name) {
    fstream f(file_name.c_str(), ios::out | ios::trunc);
    if (!f.good()) {
        return (false);
    }
    f << srvidToString(getServerID());
    f.close();
}

(btw, the bool part should be in a separate line per coding
guideline, inherited from BIND 9)

There were several of this type of warnings, also in dhcp6.

But this should have been noticed much earlier and automatically by
buildbots, which leads to another issue: -Werror is effectively
disabled due to this:

if USE_CLANGPP
# Disable unused parameter warning caused by some of the
# Boost headers when compiling with clang.
b10_dhcp4_CXXFLAGS = -Wno-unused-parameter
endif

I'd first suggest re-checking if this -Wno-xxx is still necessary;
often it's a result of naive copy-paste and actually unnecessary.
If it's still necessary, b10_dhcp4_CXXFLAGS should have $(AM_CXXFLAGS):

b10_dhcp4_CXXFLAGS = $(AM_CXXFLAGS)
if USE_CLANGPP
# Disable unused parameter warning caused by some of the
# Boost headers when compiling with clang.
b10_dhcp4_CXXFLAGS += -Wno-unused-parameter
endif

Subtickets

Change History (9)

comment:1 Changed 7 years ago by stephen

  • Milestone changed from New Tasks to Sprint-DHCP-20130214

comment:2 Changed 7 years ago by tomek

  • Owner set to stephen
  • Status changed from new to assigned

I just created branch and pushed changes. Unfortunately, the code does not build using clang on my machine. The first error is:

make[6]: Wejście do katalogu `/home/thomson/devel/bind10/src/lib/server_common/tests'
  CXX    run_unittests-run_unittests.o
  CXX    run_unittests-client_unittest.o
  CXX    run_unittests-portconfig_unittest.o
  CXX    run_unittests-keyring_test.o
  CXX    run_unittests-socket_requestor_test.o
client_unittest.cc:66:5: error: template argument uses unnamed type [-Werror,-Wunnamed-type-template-args]
    EXPECT_EQ(IPPROTO_UDP, client4->getRequestSourceEndpoint().getProtocol());
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/thomson/devel/gtest-1.6.0/include/gtest/gtest.h:1847:23: note: expanded from:
  EXPECT_PRED_FORMAT2(::testing::internal:: \
                      ^
/home/thomson/devel/gtest-1.6.0/include/gtest/gtest_pred_impl.h:162:23: note: expanded from:
  GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
                      ^
/home/thomson/devel/gtest-1.6.0/include/gtest/gtest_pred_impl.h:147:17: note: expanded from:
  GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2),\
                ^
/home/thomson/devel/gtest-1.6.0/include/gtest/gtest_pred_impl.h:77:52: note: expanded from:
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                   ^~~~~~~~~~
/usr/include/netinet/in.h:32:1: note: unnamed type used in template argument was declared here
enum
^

Many (around 10 screens) of similar errors follow. I used clang++ 3.0, which is the standard version available in Ubuntu 12.04 64 bit.

Reassigning to Stephen as agreed over Jabber.

comment:3 Changed 7 years ago by stephen

  • Status changed from assigned to reviewing

Reviewed commit 53dec20f027b589c2202f182a01d8e33ef15c2a9

I built the code with clang 4.1 on a Mac and needed to make some changes (which I've pushed) to get rid of warnings:

  • The removal of the -Wno-unused-parameters flag was reversed. Without this, the clang compilation failed in one of the boost files.
  • The SERVER_ID_FILE declaration was moved from dhcp{4,6}_srv.h to dhcp{4,6}_srv.cc to eliminate an "Unused Variable" warning/error when compiling one of the other source files in which this header file was indirectly included.
  • Two member variables in a class in src/bin/dhcp{6,4}/config_parser.cc were re-ordered to avoids a warning/error along the lines of "X will be initialized after Y"
  • A few unused arguments were commented out so that g++ compilation succeeded.

A complete rebuild with clang 4.1 on OS X 10.8 succeeded (as did a rebuild with g++ 4.6.1 on Ubuntu 11.10).

comment:4 Changed 7 years ago by stephen

  • Owner changed from stephen to UnAssigned

comment:5 Changed 7 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:6 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

Changes look ok. I also confirm that they build successfully on Ubuntu 12.04 with g++ 4.6.3.

I must admit that I was not aware that the following notation is valid:

void function(bool = false) {
  ...
}

Interesting.

Ok, code is ready for merge.

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

I must admit that I was not aware that the following notation is valid:

void function(bool = false) {
 ...
}

Nor was I until I made that change.

However, on re-checking the changes before merging, I noticed that it is not needed. It was added to comment out an unused argument - but the reason the argument was unused was a typo in the function. This typo has been corrected and the argument reinstated.

comment:8 Changed 7 years ago by tomek

Good catch. Your last change looks good.

comment:9 Changed 7 years ago by stephen

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.