On Thu, Jan 12, 2012 at 8:40 AM, Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung@gmail.com> wrote:
On Wed, Jan 11, 2012 at 9:29 AM, Dan Ivy <danivy.mail@gmail.com> wrote:
Hi,
I was recently trying out Boost.Move and a few issues worth sharing surfaced:
1. It would be helpful to have configuration macros to force emulation mode, even on C++11 compilers, as well as to disable move semantics altogether (that is, the conversion operators to boost::rv& should be disabled, and move/forward should return lvalue-references. BOOST_RV_REF and friends should remain intact, so that overloads remain unique.) In many cases, the higher level semantics of a program are expected to be identical under all three configurations, so having a quick way to switch between them is useful during testing/debugging.
It could be useful to force emulation mode, I suppose, but disabling move semantics altogether could easily break code that uses moveable-but-noncopyable types. So I question the utility of the latter, at least.
Forcing emulation mode is useful for people that primarily use compilers where true rvalue-rferences can't be disabled, such as MSVC 10+. It wouldn't set a precedence, anyway. Boost.TypeOf, for example, already has such a macro, for similiar reasons, I suppose. As for disabling move-semantics, I don't feel too strongly about this, and personally never really needed it, yet, but I forsee it as coming in handy at times. You are correct that it will break some code, this is what I was refering to by "in many cases", but move-indifferent code is probably the more common one (after all, rvalue-refs are a relatively recent feature.) I don't think it's too harmful to just have it there, anyway.
2. Boost.Move is a little bit too opaque, as it stands. What's really
missing are Boost.Move-aware type traits. Things like add_rvalue_reference are often necessary to calcuate return types of move-aware generic functions, and so on. Whether this belongs in Boost.TypeTraits or Boost.Move is a separate question. Likewise, there should be type traits to calculate the return types of boost::move and, in particular, boost::forward. On C++11, the return type of forward<T> conincides with add_rvalue_reference<T>, but not so in emulation mode, hence the necessity for this trait.
+1; perhaps Boost.TypeTraits can update the relevant metafunctions to account for boost::rv<T>&? I think we definitely should have
add_reference add_rvalue_reference add_lvalue_reference remove_reference remove_rvalue_reference remove_lvalue_reference is_reference is_rvalue_reference is_lvalue_reference
where boost::rv<T>& and (probably) boost::rv<T> const & are treated as T&&.
One thing I forgot to bring up is how these traits shoud handle types without move emulation. While it would be nice for the result of add_rvalue_reference to be aligned with the return types of boost::move and boost::forward, that is, const T&, for non-emulated types (which also does the reference-collapsing trick), it wouldn't play very nicely with the rest of the traits. (i.e. add_rvalue_ref<T> is const T&, is is_rvalue_ref<const T&> true?) This is another good reason to provide separate traits to calculate the return types of move and forward.
3. For some reason, the emulated boost::move is written so it doesn't accept temporaries. This doesn't play too nicely with forwarding: Unavoidably on C++03, there has to be made a choice whether forwarding functions use non-const or const-qualified references (bad wording, but you get the point), where the former accepts modifiable lvalues and rejects temporaries, and the latter does the opposite (well, sort of). Boost.Move chooses the latter. This means that while BOOST_FWD_REF parameters accept temporaries, these temporaries are bound as lvalues, which pretty much defeats the purpose of the library (note that these temporaries aren't eligible for copy-elision, since they're bound to a reference.)
I wouldn't use such strong language. It's "just" more limited than if true rvalue references were available. You can still pass explicitly created emulated rvalue references, and the library will allow you to move them.
It's not "just" more limited. It's very likely, the way things are now, that someone would pass a "raw" (read un-moved) temporary to a forwarding function, unaware that it wouldn't be moved (because things "just work"). It's even more likely that having been aware of that, he would prefer passing the argument through move(), as ugly as it may be. This is how I see it, anyway. And, as said, boost::move doesn't accept temporaries, so you'll have to make a lvalue out of your temporary, and only then move it. Aside from being even uglier than using move() directly, how would you do that in initializer-lists, for example? (Well, you could use some move_temp, or move(temp()) functions, but, you know, how is this any better?)
While
there's not much you can do about it on the callee-side, callers could use boost::move on the temporaries to force them into rvalue-refs. Only that they can't. This creates a somewhat confusing situation where forwarding functions are able to take temporaries, but they won't move them, while the only things that can actually be moved are lvalues...
Okay, when you phrase it that way, it is confusing :)
Ultimately, this is what I suggest: First of all, have boost::move accept temporaries.
Using a T const & parameter? I might prefer to remain safe to prevent accidentally moving lvalues-to-const.
Agreed. I overlooked this.
Secondly, seriously
consider changing the definition of BOOST_FWD_REF(T) from const T& to T&, as this would acheive two desirable, IMO, goals:: a. Forwarding functions would accept modifiable lvalues (and keep them modifiable). b. Forwarding functions would REJECT temporaries, UNLESS they're passed through boost::move, which assures that they're treated as rvalues.
This makes the use of the interface defined using BOOST_FWD_REF more onerous, *even* if true rvalue references are available! So I would consider this inferior to the current solution, even if the current solution is more limited. And, sometimes, you don't even know if the expression you're using as a parameter to a function of said interface is an rvalue or an lvalue (and sometimes it could be either, if this is all in a template).
Yes it's uglier (but not THAT ugly). Tough luck, portability comes at a price. I'm not sure what you mean by "even if true rvalue references are avialable"? The change I propose only has to do with emulation. On C++11, FWD_REFs are still T&&, so if you're using C++11 exclusively, no harm done, you can pass temporaries as usual. The only difference is that once you switch to emulation mode, you'll get errors for those cases, instead of unknowingly regressing to copy-semantics. And, as you said yourself, some types are movable-but-non-copyable. So uncautious passing of temporaries can already compile fine on C++11 and break on emulation. As for the the second point, of not knowing the l/rvalue-ness of an expression, I agree. Again, overlooked.
I sympathize with your goal, though. I've actually played around with various modifications of Boost.Move during its development process, and I made some suggestions for additions to Boost.Move which were ultimately rejected to keep the library simple (which is totally understandable). Two such suggestions were:
- An additional macro BOOST_FWD2_REF( T ) which expanded to T& (in emulation mode) and would be used only to capture lvalues or explicitly created (emulated) rvalue references. This included the result of boost::forward.
I though about suggesting something like this, but came to the same conclusion that it overly-complicates stuff. You really don't want to have different types of forwarding functions, and having to know which is which. It's best to have a single one, however unpleasent the consequences are.
- An additional macro BOOST_MOVE( expr ) which expanded to an expression nearly functionally identical to expr except it would explicitly cast the result to an emulated rvalue reference if expr was an rvalue of moveable type.
+1; I see where you're going with this. I think that, given the issues you noted with boost::move taking a reference-to-const, and not knowing the r/l-ness of an expression ahead of time, it's a good solution. Why was it rejected?
A few notes on the implementation:
4. There are many one-liner functions floating around that aren't declared inline. This is a big-deal for less-capable compilers, such as Sun and older versions of GCC. In fact, they simply won't inline boost::move calls without it.
Sounds like you should file trac tickets and, possibly, patches.
5. boost::rv<T> unconditionally inherits from T, with the assumption
that it would never get instanciated for non-class types, since it is only ever used as a reference. This is a false assumption in general. In the context of overload-resolution, the compiler is allowed, though not required, to instanciate the types of function parameters, even if the best overload can be determined without doing this. Consider this move-aware, but not very useful vector class:
#include <boost/move/move.hpp>
template <typename T> struct vector { void push_back(const T&) {} void push_back(BOOST_RV_REF(T)) {} };
int main() { vector<int> v; v.push_back(123); }
The last line of main could potentially instanciate rv<int>, breaking the program. And it does. Sun falls right into this trap. It doesn't take much to make it break on GCC either, for the same reason. Fortunately, the fix is trivial: specialize boost::rv correctly for non-class types.
Hmmm...again, probably best to file a trac ticket, with patch if possible.
All comments refer to the 1.48 release. I don't know what's going on on trunk. .
I haven't check recently, either.
- Jeff
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
I will file the tickets sometime. Not sure I'm gonna have time to write patches, though