Move support for API classes #199

ghost · 2018-04-16T12:38:55Z

I used the default keyword for implementation of move ctor and assignment. Currently copy ctor and assignment op have user defined, as it is necessary for pre c++11. I also thought this doesn't require us to include <utility> to use std::move fn(not sure whether this point is really useful). Let me know whether this is fine.

Please review when you have time. Thanks!

Can you comment on the commit boundaries I formed? Just curious to know whether I can improve on that.

ghost · 2018-04-16T13:00:49Z

xapian-bindings/xapian-headers.i

@@ -54,6 +54,7 @@
    %ignore NS::CLASS::CLASS(Internal*);
    %ignore NS::CLASS::CLASS(Internal&);
    %ignore NS::CLASS::operator=;
+    %ignore NS::CLASS::CLASS(CLASS &&);


Without this change, I get the following error.

Warning 509: Overloaded method Xapian::Document::Document(Xapian::Document const &) effectively ignored, Warning 509: as it is shadowed by Xapian::Document::Document(Xapian::Document &&).

I don't know much about SWIG. From what's already there, I added this fix. Please tell whether this is the way.

That seems appropriate to me - these objects are reference counted handles, so it's not useful to explicitly wrap ways to create a copy in the target language.

For some reason Tcl needs the copy ctor wrapped. I'm not sure we bothered to work out why, but it shouldn't need the move ctor since it doesn't currently have it.

This includes changes to PositionIterator, PostingIterator, TermIterator and ValueIterator classes. See https://trac.xapian.org/ticket/642

ghost · 2018-04-16T20:18:54Z

xapian-core/api/positioniterator.cc

-PositionIterator::PositionIterator(PositionIterator &&) = default;
+PositionIterator::PositionIterator(PositionIterator && o)
+    : internal(o.internal)
+{


Should I add LOG same as copy ctor & assignment op(except object pointer)?

I don't think logging is all that useful for these. The compiler is allowed (and sometimes required) to eliminate calls to them, so logging wouldn't correspond to the source exactly anyway.

The only reason I can think that logging might be interesting is to try to find where copies aren't eliminated to try to address that. But by inlining we open things up for the compiler to optimise in more ways, and without the programmer having to study logs. The savings aren't likely to be very large anyway, as copying just copies a pointer and adjusts reference counts.

Overall having these inline in the API headers seems a better trade-off than putting them into the library so they can have debug logging.

Member of iterator class is raw pointer, as a result compiler generated move ctor and assignment leads to memory leak, Hence providing user implementation.

ojwb · 2018-04-17T22:53:23Z

xapian-core/api/positioniterator.cc

+{
+    if (this != &o)
+	swap(internal, o.internal);
+    RETURN(*this);


This should just be return *this; - only use RETURN if there's debug logging of the function/method call (i.e. LOGCALL() or similar).

(These methods aren't really interesting to log, so I think it's fine not to have logging for them.)

ojwb · 2018-04-17T22:59:38Z

xapian-core/api/positioniterator.cc

+    : internal(o.internal)
+{
+    o.internal = nullptr;
+}


This should be OK to have inline in the public API header (include/xapian/positioniterator.h).

That's beneficial for a trivial implementation like this one as it means that the compiler can inline it and optimise the result. It also means we don't create another external symbol in the shared library (the fewer external symbols there are, the quicker it loads). One extra isn't a big deal by itself, but where they can be easily avoided we try to.

Thanks for the tip. Done.

ojwb · 2018-04-17T23:18:18Z

xapian-core/api/positioniterator.cc

+PositionIterator::operator=(PositionIterator && o)
+{
+    if (this != &o)
+	swap(internal, o.internal);


This is probably also a candidate for having inline in the public API header (and then the compiler should usually be able to at least optimise away the (this != &o) check entirely as it can typically see that the two objects are distinct).

If we do that I'm not sure about the use of std::swap() though - we don't currently pull in <utility> from public headers and the fewer library headers we include from public headers the better in many ways - user code can end up implicitly relying on our including them so removing them later risks breaking such user code. So perhaps it would be better to write it out in this case (auto t = internal; internal = o.internal; o.internal = t;).

Or alternatively leave o with NULL internals:

if (this != &o) { if (internal) decref(); internal = o.internal; o.internal = nullptr; }

(And similarly for the other iterator classes...)

ojwb · 2018-04-17T23:21:04Z

xapian-core/include/xapian/database.h

@@ -29,6 +29,7 @@

 #include <iosfwd>
 #include <string>
+#include <utility>


Ah, so we'll need <utility> for std::move anyway.

We can use static_cast to avoid it. should I go for that?

But I guess it is not a straightforward cast. I have to understand it better first.

Custom impl of move is not trivial it seems. Please ignore my comment.

The cast would be static_cast<PositionIterator&&>(obj) (when obj's class is PositionIterator).

I think that's worse than just including <utility> though - the code is much clearer using std::move().

ojwb · 2018-04-17T23:22:07Z

xapian-core/include/xapian/database.h

+    /// Move constructor.
+    WritableDatabase(WritableDatabase&& o) : Database(std::move(o)) {}
+
+    /// Move Assignment operator.


No reason to capitalise "Assignment" here.

ojwb · 2018-04-17T23:28:06Z

I used the default keyword for implementation of move ctor and assignment. Currently copy ctor and assignment op have user defined, as it is necessary for pre c++11. I also thought this doesn't require us to include to use std::move fn(not sure whether this point is really useful). Let me know whether this is fine.

I think default is better - it's shorter, clearer, and there's no scope for a slightly incorrect attempt at a default implementation. It'd be good to change the default-equivalent implementations now that we require C++11 in both the library and public headers.

Avoiding including <utility> from the public API headers would be a bonus, but I think it's not something we can easily avoid entirely.

It'd also be good to switch to using delete where we currently just make them private as that helps the compiler give better error messages.

Can you comment on the commit boundaries I formed? Just curious to know whether I can improve on that.

I just looked through the combined diff so far. I really should get on with something else right now, but I'll have a look when I get a chance.

This is beneficial because compiler can inline it and optimise the result. Also this means, less external symbol in the shared library hence loads quicker.

It is beneficial because compiler can inline it and optimise the result. Also this means, less external symbol in the shared library, so loads quicker. The implementation is chosen to avoid swap so custom swap impl or <utility> header inclusion not required.

It's short, clear and error-safe for default implementation.

Explicitly defaulted function requires return type as reference in function signature.

ghost · 2018-04-20T09:39:39Z

I think most of the classes in public API covered. Let me know if any other work is pending on this.

ojwb · 2018-06-06T04:04:11Z

Sorry getting back to this now.

The newer changes look good to me, will merge this now.

Can you comment on the commit boundaries I formed? Just curious to know whether I can improve on that.

They look great to me.

ojwb · 2018-06-06T04:32:34Z

git rebase -i --autosquash-ed and merged to master in 32a7fd6.

Thanks for all your work on this.

Update bindings for newly added move ctor

9aef720

ghost commented Apr 16, 2018

View reviewed changes

guruhegde added 3 commits April 16, 2018 15:16

Add move support for Database & WritableDatabase

9ceec82

Add move support for Document & TermGenerator class

679633b

Add move support for Enquire class

47feab0

ghost changed the title ~~Move support for API classes having intrusive_ptr_nonnull as member~~ Move support for API classes having only smart pointer as member Apr 16, 2018

guruhegde added 6 commits April 16, 2018 15:34

Add move support for ESet, MSet & RSet classes

8284718

Add move support for query, queryparser & stem class

b486a00

Add move support for Registry class

b5a0019

Add move support for Cluster, ClusterSet & DocumentSet

3ec32a7

Add move support for iterator classes.

4ebed1f

This includes changes to PositionIterator, PostingIterator, TermIterator and ValueIterator classes. See https://trac.xapian.org/ticket/642

fixup! Add move support for Registry class

f448baa

ghost commented Apr 16, 2018

View reviewed changes

guruhegde added 2 commits April 16, 2018 22:23

fixup! Add move support for query, queryparser & stem class

1b0e158

Implement move ctor and assignment for iterator classes.

423551a

Member of iterator class is raw pointer, as a result compiler generated move ctor and assignment leads to memory leak, Hence providing user implementation.

ojwb reviewed Apr 17, 2018

View reviewed changes

guruhegde added 7 commits April 18, 2018 12:37

Inline Move ctor for iterators in public API header

1c679ce

This is beneficial because compiler can inline it and optimise the result. Also this means, less external symbol in the shared library hence loads quicker.

fixup! Add move support for Database & WritableDatabase

ae1ff56

Explicitely default copy ctor and assignment for API classes

cb17f05

It's short, clear and error-safe for default implementation.

Declare copy ctor and asgmt as deleted for non-copyable types

eb562e0

Change return type in copy asgmt fn for DocumentSet

02aeb2c

Explicitly defaulted function requires return type as reference in function signature.

fixup! Explicitely default copy ctor and assignment for API classes

c9226c9

ghost changed the title ~~Move support for API classes having only smart pointer as member~~ Move support for API classes Apr 24, 2018

ojwb closed this Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move support for API classes #199

Move support for API classes #199

ghost commented Apr 16, 2018 •

edited by ojwb

ghost Apr 16, 2018

ojwb Apr 17, 2018

ghost Apr 16, 2018

ojwb Apr 18, 2018

ojwb Apr 17, 2018

ghost Apr 18, 2018

ojwb Apr 17, 2018

ghost Apr 18, 2018

ojwb Apr 17, 2018 •

edited

ojwb Apr 17, 2018

ghost Apr 18, 2018

ghost Apr 18, 2018

ghost Apr 18, 2018

ojwb Apr 18, 2018

ojwb Apr 17, 2018

ghost Apr 18, 2018

ojwb commented Apr 17, 2018

ghost commented Apr 20, 2018

ojwb commented Jun 6, 2018

ojwb commented Jun 6, 2018

Move support for API classes #199

Move support for API classes #199

Conversation

ghost commented Apr 16, 2018 • edited by ojwb

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojwb Apr 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojwb commented Apr 17, 2018

ghost commented Apr 20, 2018

ojwb commented Jun 6, 2018

ojwb commented Jun 6, 2018

ghost commented Apr 16, 2018 •

edited by ojwb

ojwb Apr 17, 2018 •

edited