khuttun / polym Goto Github PK

Hi,

First thank you for the hard work. Very interesting to see clean code with pimpl.

I have few suggestions for your library though. Each of them deserve a PR I can make.

1. PolyM/Queue.hpp

   Lines 30 to 38 in f97db66

   	/**
   	* Get message from the head of the queue.
   	* Blocks until at least one message is available in the queue, or until timeout happens.
   	* If get() returns due to timeout, the returned Msg will have Msg ID MSG_TIMEOUT.
   	*
   	* @param timeoutMillis How many ms to wait for message until timeout happens.
   	* 0 = wait indefinitely.
   	*/
   	std::unique_ptr<Msg> get(int timeoutMillis = 0);

   
   Why not returning an empty pointer instead of forging a new message?
   This could allow returning nullptr thus avoiding pushing then poping from the queue here:

   PolyM/Queue.cpp

   Lines 44 to 50 in f97db66

   	if (timeoutOccured)
   	queue_.emplace(new Msg(MSG_TIMEOUT));
   	}
   	auto msg = queue_.front()->move();
   	queue_.pop();
   	return msg;

2. I think a try_get could be really useful:

/**
    * Get message from the head of the queue. Returns an empty pointer if no 
    * message is available.
    */
    std::unique_ptr<Msg> try_get(); 

3. PolyM/Queue.cpp

   Lines 57 to 59 in f97db66

   	auto it = responseMap_.emplace(
   	std::make_pair(msg.getUniqueId(), std::unique_ptr<Queue>(new Queue))).first;
   	lock.unlock();

   
   I find this a bit too overkill to recreate a new message queue. But I think there are not other design for this?

Thank you again for this wonderful piece of code!
BR

it s became the foundation for message passing , it s work really great!!

An idea to greatly improve performance would be a CircularQueue type.
Performance of circular buffer is an order of magnitude faster that the deque container used by STL (underlying container of std::queue) as per the following benchmark (consider case with vector with reserve() as the vector is pre-allocated):
Benchmark

One example is the use in the spdlog lib : spdlog

Its using a fixed size std::vector and add the interface for circular buffer around it.

Would it be possible to mark move operations in Msg and DataMsg deleted and implement the moving by hand in the virtual move() functions?

Hi,

I'm having a problem with using DataMsg template nested inside another DataMsg, like this: DataMsg<DataMsg<int>>

My use case is to extract the nested DataMsg and pass it on to another class. As I do not know the actual data type of DataMsg yet, I'm starting by casting to the base Msg type like this:

function(Msg &base) {
    dynamic_cast<DataMsg<Msg>&>(base)
}

However, this cast fails with a bad cast exception. Doing the same using a static cast seems to work as expected (payload OK, no segfault).

From a polymorphic view point I can't see anything wrong with my approach. As dynamic casting works for the non-nested type, it should work for the nested one too?

Any ideas?

Thanks!

I've attached a minimalistic test program that shows the problem. This uses the Msg and DataMsg type with everyting removed that is not essential.
Build and run with
g++ test.cpp -o test.x && ./test.x

test.zip

Hello,
it is not an issue it s only a message to say that the impl is really good, thanks you.
regards,
david G.