Le 13/06/13 01:07, Klaim - Joël Lamotte a écrit :
Thanks for your patience.
On Wed, Jun 12, 2013 at 10:47 PM, Vicente J. Botet Escriba < vicente.botet@wanadoo.fr> wrote:
explicit Sequence( SequenceId new_id, SequenceInfo info )
explicit is not needed here.
In C++11 it prevent returning a temporary with more than one attributes in the constructor without using the type name explicitly:
// returning a Sequence return { sequence_id, some_info }; // error (in C++11): the constructor is explicit return Sequence{ sequence_id, some_info }; // OK (C++11)
I don't have the standard around to check but here is an explanation: http://stackoverflow.com/questions/12437241/c-always-use-explicit-constructo... I was not aware of this C++11 feature. Thanks.
(note that it's just an example, in my real code Sequence is not a value semantic type)
I would define thesame kind of constructor for the class SequenceInfo .
Sequence( SequenceId new_id, SequenceInfo info ) : m_info( new_id, std::move(info) ) {}
This example is a contrived/simplistic one. In my real code the member I'm setting is from generated code which I can't modify without important efforts. Assume that I'm not in the control of that member's type definition. The best I could do in this case, I suppose, would be a function wrapper:
Sequence( SequenceId new_id, SequenceInfo info ) : m_info( merge_info( new_id, info ) ) // no locking {}
Which should be equivalent to your suggestion I guess, This seems a good alternative. but it is not enough in all cases. See below.
If I am correct, then the lock of the synchronized_value is unnecessary.
You don't need to lock at construction. but once constructed the lock is imperative.
Just to clarify, and make sure I understand 100% correctly, are you talking about the member's construction (m_info) or the owning object construction (Sequence)? I meant the member. I assume the later, so the member don't need locking until the end of the constructor of the owning object. No. Any access to the member would imply a lock after construction.
m_info-> returns a lock.
I suspect that there might be other cases like this one where
the object responsable for the synchronized object should be able, rarely, to access the object unsafely.
You are supposing that there is an object responsible for synchronized object, but I don't know what this means and how to ensure it. If you want this pattern , you maybe need external locking. See the class externally_locked.
For example?
This is just speculation. Maybe I'm just pessimistic. Currently I only have the case of the constructor and I don't think I will have others. I have another bigger code base which have more complex concurrent cases where I might find similar usage (access to a usually locked object in a case where I am 100% sure there can't be concurrent accesses). Could you develop the case and explain how are you 100% sure that there is no concurrent access? I will search and report if I find something, I'm not sure I will.
Suggestion: add an unsafe access to the object, in a very visible way.
m_info.unsafe_access().id( new_id );
OR
m_info.unsafe_access()->id( new_id ); // using a smart pointer which checks if safe accesses have been done inbetween and throw if it is the case? or any other checks detecting potential error BTW, I don't know what "checks if safe accesses have been done inbetween" could mean? Could you clarify? I don't think this is needed for the use case presented. Just create the appropiated constructors.
As I was saying, in the real code I will have to use a builder function instead, I can't modify that type. This seems a good alternative.
However it is still not a perfect solution. In the real code I have some logic I need to do before manipulating the synchronized member. It should be done in this order, first some logic, then use thesynched object. But even if I use a function to create the member but still have to access it later because of that logic, there is no point in doing that. You can create a unsynchronized SequenceInfo object, do whatever you want with and last create the synchronized SequenceInfo just by copying. To clarify what I'm trying to explain (this is fictive code close to mine, which is more complex):
explicit Sequence( SequenceId new_id, SequenceInfo info ) : m_info( merge_info( new_id, info ) ) // assuming I follow your suggestion { if( first_thing_to_do() ) { second_thing_to_do( m_info->name ); // still need to lock } } I recognize that the factory function could be less natural, but it works
I'm using function name instead of real code just so you get the idea, sorry if it's not real code. I try to do minimum work in the constructor but I noticed the locking access when I couldn't avoid it.
I will try to be clearer with some code, first this:
class InfoProcessor; // generate analytic data from info - non-copyable! Is it movable?
class Sequence { boost::synchronized_value<InfoProcessor> m_infoproc;
public: explicit Sequence( SequenceInfo info ) : m_infoproc( info ) // process info into the constructor, ready to be used Isn't the parameter InfoProcessor info? { m_infoproc->do_something(); // can't avoid a lock? }
Sequence( const Sequence& ) = delete; Sequence& operator=( const Sequence& ) = delete;
Assuming I can't modify InfoProcessor, the only way I see how to do the call in the constructor without locking would be to encapsulate the member into a wrapper:
class Sequence { struct InfoProcWrapper { InfoProcWrapper( SequenceInfo info ) : infoproc( info ) { infoproc.do_something(); } // no locking
InfoProcessor infoproc; }; boost::synchronized_value<InfoProcWrapper > m_infoproc;
public: explicit Sequence( SequenceInfo info ) : m_infoproc( info ) // now any access will need locking { // OK: m_infoproc->infoproc.do_something() was already called without locking. }
Which would be fine if it wasn't so noisy and that's only for 1 synchronized member... I agree, creating these wrappers is too noisy but an alternative. I
SequenceInfo makeIt( SequenceId new_id, SequenceInfo info ) { info = merge_info( new_id, info ); if( first_thing_to_do() ) { second_thing_to_do( info->name ); // still need to lock } } return info } explicit Sequence( SequenceId new_id, SequenceInfo info ) : m_info( makeIt( new_id, info ) ) {} think the factory is a better choice. Note that you have lambdas in C++11.
class Sequence { struct InfoProcWrapper { InfoProcWrapper( SequenceInfo info ) : infoproc( info ) { infoproc.do_something(); } // no locking
InfoProcessor infoproc; }; struct DataCruncherWrapper { DataCruncherWrapper( SequenceInfo info ) : cruncher( info ) { cruncher.prepare( 42 ); } // no locking
DataCruncher cruncher; }; boost::synchronized_value<InfoProcWrapper> m_infoproc; boost::synchronized_value<DataCruncher> m_cruncher; // should NOT be synched with the same mutex
public: explicit Sequence( SequenceInfo info ) : m_infoproc( info ) // OK: setup with no locking : m_cruncher( info ) // OK: setup with no locking { m_cruncher->cruncher.configure( m_infoproc->infoproc ); // how to do that without locking? }
Delaying the construction. This is similar to the problems raised for writing constexpr fuctions. class Sequence { struct X { InfoProcWrapper m_infoproc; DataCruncher m_cruncher; X(SequenceInfo info) : sx_( info ) : m_cruncher( info ) { m_cruncher->cruncher.configure( m_infoproc->infoproc ); } }; struct SX { boost::synchronized_value<InfoProcWrapper> m_infoproc; boost::synchronized_value<DataCruncher> m_cruncher; // should NOT be synched with the same mutex SX(X) {...}; }; SX sx_; public: explicit Sequence( SequenceInfo info ) : sx_( X(info) ) { }
void crunch( Data data ) { m_cruncher->cruncher.crunch( data ); } // OK: locks only one mutex void add( Element element ) { m_infoproc->infoproc.add( element ); } // OK: locks only one mutex
Anyway, I might be wrong but it seems to me that any attempt to combine use of several synched members in the constructor will imply locking OR adding several layers of abstractions, which if the members' types have value-semantic, can be easy, but if it's not the case, it becomes quickly very noisy.
I agree. We surely need something that make this easier and less noisy. I would prefer if we continue to see other alternatives to go directly to providing a bypass function.
Obviously it is still just speculation but it don't seem unfamiliar to have this kind of work in a constructor.
It looks like it would allow avoiding cost where it could be avoided using
a mutex manipulated manually.
Any thoughts on this?
I'm open to your suggestion if you have other use cases that can not be solved otherwise.
I will have to find a concrete and more convincing use case and report here then.
Note that hopefully the cost of the lock isn't important so far in my specific use case of today, I was just pointing the constructor cost because in the other code bases I have I might have more performance-related concerns and I might not be the only one.
I would help you if I can. HTH, Vicente