Opened 7 years ago

Closed 7 years ago

#2764 closed defect (fixed)

base_xx wrapper doesn't compile with Boost 1.53

Reported by: jinmei Owned by: jinmei
Priority: high Milestone: Sprint-20130319
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: 2 Add Hours to Ticket: 0
Total Hours: 3.49 Internal?: no

Description

The latest release version of Boost (1.53) introduced some non
trivial changes to classes we are using for base-32/64 and hex
conversions, and it breaks build and tests of BIND 10.

The key issues are:

  • to meet the requirement of transform_width, we need the postfix version of operator++ for EncodeNormalizer
  • binary_from_base64.hpp now recognizes the padding character, which breaks our assumption on some failure mode.
  • transform_width does not call redundant operator* beyond the end of the iterator. It seems to be safe and good itself, but our code relies on it to catch an error case ourselves. So it now doesn't work as intended.

A quick hack patch that is attached will fix these issues. We'll need
to clean it up, review it, and merge.

p.s. I suspect the need for the postfix version of operator++ is a
kind of regression and reported it:
https://svn.boost.org/trac/boost/ticket/8081

Subtickets

Attachments (1)

base_n.diff (3.5 KB) - added by jinmei 7 years ago.

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by jinmei

comment:1 Changed 7 years ago by jinmei

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed
  • Priority changed from medium to high

comment:2 Changed 7 years ago by jaspain

Successfully tested the patch on Ubuntu 12.10 with Boost 1.53.0. Referencing the boost ticket above, it looks like the boost people consider their change that broke BIND 10 to be a "feature". Thanks. Jeff.

comment:3 Changed 7 years ago by jelte

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

comment:4 Changed 7 years ago by jinmei

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

comment:5 Changed 7 years ago by jinmei

trac2764 is ready for review.

I've applied the originally proposed patch with some cleanups and
documentation update. After a full check for unit tests I found one
regression, so I also fixed that in the last commit.

Proposed changelog:

584.?	[bug]		jinmei
	Fixed build failure with Boost 1.53 (and probably higher) in the
	internal utility library.  Note that with -Werror it may still
	fail, but it's due to a Boost bug that is reportedly fixed in their
	development trunk.  See https://svn.boost.org/trac/boost/ticket/8080
	Until the fix is available in a released Boost version you may need
	to specify the --without-werror configure option to build BIND 10.
	(Trac #2764, git TBD)

(the "note that" part is not directly related to the subject of this
ticket, but I think it's worth noting).

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 vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 0.49

I noticed only one minor point:

if (((char_count * BitsPerChunk) & 7) != 0) {

I think it may be easier to read with % 8 instead of & 7, especially when looking at the comment above. Note that any compiler will just change that to the mask internally instead of using division instructions.

After that, I believe it can be merged.

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

Thanks for the review.

Replying to vorner:

I noticed only one minor point:

if (((char_count * BitsPerChunk) & 7) != 0) {

I think it may be easier to read with % 8 instead of & 7, especially when looking at the comment above. Note that any compiler will just change that to the mask internally instead of using division instructions.

Hmm, I'm not so sure about "any compiler" - I'm afraid the level of
arithmetic reduction can be quite compiler dependent except for very
trivial ones. But from quick experiments it at least seems to be the
case for clang++ and g++, so I made the change. BaseX decoding is
relatively expensive anyway, so even if it's not optimized it's
probably not a big deal.

After that, I believe it can be merged.

I found one other point of cleanup, but I believe it's trivial, so
I've merge the branch with that cleanup. Now closing the ticket.

comment:10 Changed 7 years ago by jinmei

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