Opened 10 years ago

Closed 6 years ago

#642 closed defect (fixed)

Add C++11 move constructors and assignment ops

Reported by: Olly Betts Owned by: Olly Betts
Priority: normal Milestone: 1.4.6
Component: Library API Version:
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Operating System: All

Description

When compiling in C++11 mode, we should provide these where they are useful.

Taking care of the public API is probably more of a priority than internal uses, as it would be better to have that done before we release 1.4.0, though if these new methods are inlined this probably isn't an ABI-incompatible change.

For intrusive_ptr, newer BOOST seems to have suitable versions for us to copy over.

Change History (9)

comment:1 by Olly Betts, 8 years ago

Milestone: 1.3.x1.4.x

Probably not an ABI breaking change, and if it is it can just wait for the next stable release series.

comment:2 by Olly Betts, 6 years ago

Keywords: GoodFirstBug added

comment:3 by Guruprasad Hegde, 6 years ago

Patch for move support for smart pointers: https://github.com/xapian/xapian/pull/197

I would like to work on adding move support for other classes as well. Any strategy or points to keep in mind? Is it required to add test cases for these?

comment:4 by Olly Betts, 6 years ago

Owner: changed from Olly Betts to Guruprasad Hegde

It'd be good to extend this to the other classes. I'm not sure there's anything special to bear in mind. A sensible approach is probably to tackle the classes in groups that are similar - e.g. classes which inherit from intrusive_base, those which inherit from opt_intrusive_base, the iterator classes, and anything else.

A simple test case would probably be good - there are generated testcases for the API classes which is probably the place to do that - see tests/generate-api_generated. That way you probably only need to write one or two testcases in template form.

comment:5 by Olly Betts, 6 years ago

Milestone: 1.4.x1.5.0

Move ctor and assignment op for intrusive_ptr merged to git master in 9b2ffb11adfd4784365837a5fae2da7c8de8c68f.

I think at this point in the 1.4.x release cycle it isn't sensible to backport these changes - the next stable release branch is coming soon and we should be getting more conservative about changes that go into 1.4.x, so updating milestone to reflect that.

Backporting would also need a lot of conditionalisation as 1.4.x supports using a non-C++11 compiler to build code that uses Xapian, public headers can't unconditionally use C++11 features.

comment:6 by Olly Betts, 6 years ago

I've pushed a commit which extends the generated apitest testcases to check that std::move works on API classes which are meant to be copyable (98c0e0d3f7778ec20ab7e62f67c2edc03aa392f7). This doesn't attempt to check if the object is actually moved (that seems hard to do without poking at the internals) but it at least checks that any move constructor/operators that we add compile and pass a basic sanity check.

comment:7 by Guruprasad Hegde, 6 years ago

Patch in progress for API classes: https://github.com/xapian/xapian/pull/199

comment:8 by Olly Betts, 6 years ago

Keywords: GoodFirstBug removed
Milestone: 1.5.01.4.6
Owner: changed from Guruprasad Hegde to Olly Betts
Status: newassigned

Addressed in git master in 32a7fd61a43f4e817578a462e55c53406f68e4ac.

As above, this is not trivial to backport, though Kronuz spotted that move constructors would help with thread safety in his use case - https://botbot.me/freenode/xapian/2018-05-01/?msg=99592697&page=1 - I've asked him for more info on IRC just now. Setting milestone at 1.4.6 so we can make a definite call on this.

comment:9 by Olly Betts, 6 years ago

Resolution: fixed
Status: assignedclosed

I've not heard back from Kronuz, but I had a go at backporting and making this all conditional and it wasn't too hard. Pushed as a series of commits, ending with 3191785e4ef7fcfc964108a14acdeebe718d86d5.

Note: See TracTickets for help on using tickets.