Opened 11 years ago

Closed 11 years ago

#979 closed Bugs (fixed)

::boost::detail::empty_base could be improved

Reported by: sparent@… Owned by: Daniel Frey
Milestone: Boost 1.34.1 Component: operators
Version: Boost 1.34.0 Severity: Optimization
Keywords: operator library Cc: dave@…, witt@…, d.frey@…

Description

The empty base class used by the operators library will not actually be empty if you have a class inheriting from operators and the first member also inherits from operators - the work around is to have an empty_base<T> to ensure a unique type - so you end up with:

class my_class : equality_comparable<my_class, my_class, empty_base<my_class> > { ...

Kind of gross - such a base could be included directly in the operator library.

Sean

Attachments (1)

operators.hpp.diff (25.5 KB) - added by Thomas Witt 11 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by Daniel Frey

Milestone: Boost 1.34.1Boost 1.35.0
Owner: set to Daniel Frey
Status: newassigned

Accepted, but raised milestone to 1.35.0 as the risk of breaking other compilers is not worth it. Once regression tests show that everything is fine, I might consider moving it back to 1.34.1, but it should not delay 1.34.1.

comment:2 Changed 11 years ago by Daniel Frey

Component: iteratoroperators
Severity: ProblemOptimization

comment:3 Changed 11 years ago by Daniel Frey

Resolution: fixed
Status: assignedclosed

Fixed in CVS HEAD

comment:4 Changed 11 years ago by Thomas Witt

Milestone: Boost 1.35.0Boost 1.34.1
Resolution: fixed
Status: closedreopened

Reopened for inclusion in 1.34.1

comment:5 Changed 11 years ago by Dave Abrahams

I can't see how this is appropriate for 1.34.1

It's NAD, in my opinion, and should be put off until the next full release.

comment:6 Changed 11 years ago by sparent@…

Although I have a work around so putting off until 1.35.0 is fine with me - the work around is ugly and I do consider a helper template that adds unnecessary words to my objects a defect.

comment:7 in reply to:  5 ; Changed 11 years ago by Thomas Witt

Replying to dave:

I can't see how this is appropriate for 1.34.1

It's a "simple" change, passes testing on HEAD and it won't delay 1.34.1 even if we have to back it out again.

It's NAD, in my opinion, and should be put off until the next full release.

If this were a commercial project I would agree. My experience is that a more opportunistic approach is required for boost. There is no way prioritizing changes when you can not prioritize work items.

comment:8 in reply to:  7 ; Changed 11 years ago by Dave Abrahams

Replying to witt:

Replying to dave:

I can't see how this is appropriate for 1.34.1

It's a "simple" change, passes testing on HEAD and it won't delay 1.34.1 even if we have to back it out again.

Unless the change is not exactly as proposed in the summary of this ticket, it is a breaking change for users. I can't imagine any good excuse for doing that in a point release, and I object strongly to doing so.

Changed 11 years ago by Thomas Witt

Attachment: operators.hpp.diff added

comment:9 in reply to:  8 Changed 11 years ago by Thomas Witt

Replying to dave:

Replying to witt:

Replying to dave:

I can't see how this is appropriate for 1.34.1

It's a "simple" change, passes testing on HEAD and it won't delay 1.34.1 even if we have to back it out again.

Unless the change is not exactly as proposed in the summary of this ticket, it is a breaking change for users. I can't imagine any good excuse for doing that in a point release, and I object strongly to doing so.

Dave, I have attached the proposed patch. This may very well be my fault. I never intended to add a breaking change. Looking at the patch I still can't figure it out. What am I missing?

comment:10 Changed 11 years ago by Thomas Witt

Cc: witt@… added

comment:11 Changed 11 years ago by Dave Abrahams

Sorry, the mistake is mine. I didn't realize, from the example, which used no qualification and showed empty_base named in what is presumably user code, that empty_base was in boost::detail. As long as users aren't touching empty_base the patch is OK.

comment:12 in reply to:  11 ; Changed 11 years ago by Thomas Witt

Replying to dave:

I always assumed everything in boost::detail is fair game. Should we be more conservative?

comment:13 in reply to:  12 ; Changed 11 years ago by anonymous

Replying to witt:

Replying to dave:

I always assumed everything in boost::detail is fair game.

Well, it should be... but occasionally you see stuff like this:

http://www.boost.org/libs/serialization/doc/archive_reference.html http://www.boost.org/libs/serialization/doc/derivation.html

Should we be more conservative?

I don't think so. Did http://svn.boost.org/trac/boost/ticket/979#comment:11 lead you to think I would say otherwise.

comment:14 in reply to:  13 Changed 11 years ago by Thomas Witt

Replying to anonymous:

Replying to witt:

Replying to dave:

I always assumed everything in boost::detail is fair game.

Well, it should be... but occasionally you see stuff like this:

http://www.boost.org/libs/serialization/doc/archive_reference.html http://www.boost.org/libs/serialization/doc/derivation.html

Sigh!

Should we be more conservative?

I don't think so. Did http://svn.boost.org/trac/boost/ticket/979#comment:11 lead you to think I would say otherwise.

No, not really. I am just paranoid about misunderstandings with release issues ;-).

comment:15 Changed 11 years ago by Daniel Frey

Cc: d.frey@… added
Resolution: fixed
Status: reopenedclosed

Applied patch to RC_1_34_0. Sorry Dave, I should have added the patch to the ticket from the very beginning.

Note: See TracTickets for help on using tickets.