Opened 11 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 , 9 years ago
Milestone: | 1.3.x → 1.4.x |
---|
comment:2 by , 7 years ago
Keywords: | GoodFirstBug added |
---|
comment:3 by , 7 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 , 7 years ago
Owner: | changed from | to
---|
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 , 7 years ago
Milestone: | 1.4.x → 1.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 , 7 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 , 7 years ago
Patch in progress for API classes: https://github.com/xapian/xapian/pull/199
comment:8 by , 6 years ago
Keywords: | GoodFirstBug removed |
---|---|
Milestone: | 1.5.0 → 1.4.6 |
Owner: | changed from | to
Status: | new → assigned |
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 , 6 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.
Probably not an ABI breaking change, and if it is it can just wait for the next stable release series.