Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#2705 closed defect (fixed)

clang unused-private-field errors

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20130305
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0.33 Internal?: no

Description

I noticed a few issues with upgrading clang++ (Apple's semi-proprietary
version, seemingly based on some variant of LLVM 3.2). It now warns
about "unused private member variables", which caused the following:

  • it unexpectedly disable -Werror because this check now fails
       AC_TRY_COMPILE([namespace { class Foo {}; }
       namespace isc {class Bar {Foo foo_;};} ],,
    	[AC_MSG_RESULT(no)
    	 werror_ok=1
    	 B10_CXXFLAGS="$B10_CXXFLAGS -Werror"],
    	[AC_MSG_RESULT(yes)])
    
    We need to update the test so that it doesn't hit the "unused private" warning.
  • there's real "unused private" issue in our code, so if we enabled -Werror, build would fail anyway. At least it warns about LocalZoneData::class_ (in lib/cache), and I suspect there may be some more. We'll need to fix them.

Right now I'm disabling -Werror, but that's a bad practice and would
like to fix this as an urgent matter. I suggest pushing it to the
current sprint if we run out of open tickets, or adopting it in the next
sprint at the latest.

Subtickets

Change History (10)

comment:1 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130305

comment:2 Changed 7 years ago by jinmei

  • Priority changed from very high to medium

comment:3 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from new to accepted

I've actually done most of this task already, so I'll pick it up
to avoid duplicate effort by someone else.

comment:4 Changed 7 years ago by jinmei

trac2705 is ready for review.

you may not be confirm it "fixes" the problem unless you have this
version of clang++, but anyone should be easily able to check it
doesn't do any harm. And, I confirmed it fixed the problem in my
environment.

One possibly controversial point is how to handle DHCP's
Pool6::prefix_len_. I was not sure it was actually intended to be
used (in which case the fact it's not used is a real bug) or we can
simply remove it, so in this branch I kept the variable and actually
used it in argument validation (with a comment about the intent). I
also opened a ticket for the DHCP team. So, independently from what
we should do for this, I believe we can merge this branch.

I don't think we need a changelog entry for this change. Due to the
false positive of configure.ac (which is also fixed in this branch),
this problem actually didn't cause build failure, so the change is
basically not visible for users.

comment:5 Changed 7 years ago by jinmei

Forgot to mention: the DHCP ticket is #2789.

comment:6 Changed 7 years ago by jinmei

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

comment:7 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:8 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

as you already guessed, I haven't tried using clang myself to see if there are no more errors, but the changes do indeed look fine, and I am happy to defer the prefix_len_ to #2789, so please go ahead and merge.

comment:9 in reply to: ↑ 8 Changed 7 years ago by jinmei

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

Replying to jelte:

as you already guessed, I haven't tried using clang myself to see if there are no more errors, but the changes do indeed look fine, and I am happy to defer the prefix_len_ to #2789, so please go ahead and merge.

Thanks for the review, merge done, closing.

comment:10 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 0.33
Note: See TracTickets for help on using tickets.