Convert Queue into a template class

Convert Queue into a template class

Please find a description in this mailing list post:

http://mailman.isi.edu/pipermail/ns-developers/2017-January/013761.html

[Tom Henderson, 2017-01-15 23:38:54]

Need cc:ns-3-reviews@googlegroups.com

https://codereview.appspot.com/316080043/diff/1/src/core/model/object-base.h
File src/core/model/object-base.h (right):

https://codereview.appspot.com/316080043/diff/1/src/core/model/object-base.h#...
src/core/model/object-base.h:68: typedef type<param> type ## Of ## param
what about param ## type instead?  This would allow us to have class name
'PacketQueue' instead of 'QueueOfPacket' which IMO is more natural and
consistent with the rest of ns-3 Object type id and class names (e.g.
'NetDeviceContainer' instead of 'ContainerOfNetDevice').

We could also consider 'NS_OBJECT_TEMPLATE_CLASS_DECLARE' to be more explicit.

[Mohit P. Tahiliani, 2017-01-16 19:04:11]

+1 for the overall patch.

I've reviewed it partially and suggested a few minor corrections. I'll try to
review other parts soon.

Thanks,
Mohit P. Tahiliani

https://codereview.appspot.com/316080043/diff/1/src/network/utils/queue.h
File src/network/utils/queue.h (right):

https://codereview.appspot.com/316080043/diff/1/src/network/utils/queue.h#new...
src/network/utils/queue.h:137: * \return The total number of bytes dropped after
dequeue by this Queue
Should be "The total number of packets dropped after dequeue . . ."

https://codereview.appspot.com/316080043/diff/1/src/network/utils/queue.h#new...
src/network/utils/queue.h:446: // If DoEnqueue fails, Queue::Drop is called by
the subclass
I think this should change because we don't have Drop method now.

https://codereview.appspot.com/316080043/diff/1/src/network/utils/queue.h#new...
src/network/utils/queue.h:486: NsLog (LOG_LOGIC, "m_traceDequeue (packet)");
should we use "m_traceDequeue (p)" to make it in line with other log messages?
Except this, all use (p)

[Stefano Avallone, 2017-01-19 18:49:17]

A bit more polished version which takes comments into account. I still have to
update the documentation.

Thanks.

https://codereview.appspot.com/316080043/diff/1/src/core/model/object-base.h
File src/core/model/object-base.h (right):

https://codereview.appspot.com/316080043/diff/1/src/core/model/object-base.h#...
src/core/model/object-base.h:68: typedef type<param> type ## Of ## param
On 2017/01/15 23:38:54, Tom Henderson wrote:
> what about param ## type instead?  This would allow us to have class name
> 'PacketQueue' instead of 'QueueOfPacket' which IMO is more natural and
> consistent with the rest of ns-3 Object type id and class names (e.g.
> 'NetDeviceContainer' instead of 'ContainerOfNetDevice').
> 
> We could also consider 'NS_OBJECT_TEMPLATE_CLASS_DECLARE' to be more explicit.

Done.

https://codereview.appspot.com/316080043/diff/1/src/network/utils/queue.h
File src/network/utils/queue.h (right):

https://codereview.appspot.com/316080043/diff/1/src/network/utils/queue.h#new...
src/network/utils/queue.h:137: * \return The total number of bytes dropped after
dequeue by this Queue
On 2017/01/16 19:04:11, Mohit P. Tahiliani wrote:
> Should be "The total number of packets dropped after dequeue . . ."

Done.

https://codereview.appspot.com/316080043/diff/1/src/network/utils/queue.h#new...
src/network/utils/queue.h:446: // If DoEnqueue fails, Queue::Drop is called by
the subclass
On 2017/01/16 19:04:11, Mohit P. Tahiliani wrote:
> I think this should change because we don't have Drop method now.

Done.

https://codereview.appspot.com/316080043/diff/1/src/network/utils/queue.h#new...
src/network/utils/queue.h:486: NsLog (LOG_LOGIC, "m_traceDequeue (packet)");
On 2017/01/16 19:04:11, Mohit P. Tahiliani wrote:
> should we use "m_traceDequeue (p)" to make it in line with other log messages?
> Except this, all use (p)

Done.

[Stefano Avallone, 2017-02-10 17:58:30]

Changes in the new version:

- removed the alias (now use Queue<Packet>)
- made use of forward declarations and explicit template instantiation
declaration to optimize build performance (especially in terms of dependencies
on header files)
- fixed compilation with g++-4.9.2
- updated to latest ns-3-dev

[Tom Henderson, 2017-02-22 17:29:13]

I am fine with merging it, provided that:
- CHANGES.html outlines the user-visible API changes
- there aren't any hiccups with Python bindings generation

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.h
File src/network/utils/queue.h (right):

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.h...
src/network/utils/queue.h:66: * \param itemType the item type
consider to add an example usage around \code ... \endcode since this method is
a bit unconventional

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.h...
src/network/utils/queue.h:279: * implementation of the class Queue methods.
perhaps even more explicit, add "(i.e., do not include queue.h in your .h files
but instead use the forward declaration, and then include queue.h in your .cc
files)"

[Mohit P. Tahiliani, 2017-02-26 15:22:40]

Hi Stefano,

The changes look good to me.

I have left some minor suggestions on improving doxygen comments and logging
functions.

Regards,
Mohit P. Tahiliani

https://codereview.appspot.com/316080043/diff/60001/src/csma/model/csma-net-d...
File src/csma/model/csma-net-device.h (right):

https://codereview.appspot.com/316080043/diff/60001/src/csma/model/csma-net-d...
src/csma/model/csma-net-device.h:131: * DropTail or RED.
I think documentation should be updated here.

https://codereview.appspot.com/316080043/diff/60001/src/flow-monitor/model/ip...
File src/flow-monitor/model/ipv4-flow-probe.h (right):

https://codereview.appspot.com/316080043/diff/60001/src/flow-monitor/model/ip...
src/flow-monitor/model/ipv4-flow-probe.h:116: /// \param item queue item
\param item queue disc item

https://codereview.appspot.com/316080043/diff/60001/src/flow-monitor/model/ip...
File src/flow-monitor/model/ipv6-flow-probe.h (right):

https://codereview.appspot.com/316080043/diff/60001/src/flow-monitor/model/ip...
src/flow-monitor/model/ipv6-flow-probe.h:122: /// \param item queue item
\param item queue disc item

https://codereview.appspot.com/316080043/diff/60001/src/internet/model/ipv4-q...
File src/internet/model/ipv4-queue-disc-item.cc (right):

https://codereview.appspot.com/316080043/diff/60001/src/internet/model/ipv4-q...
src/internet/model/ipv4-queue-disc-item.cc:40: {
Should we add NS_LOG_FUNCTION here? If it was intentionally left out, then we
can leave as it is.

https://codereview.appspot.com/316080043/diff/60001/src/internet/model/ipv6-q...
File src/internet/model/ipv6-queue-disc-item.cc (right):

https://codereview.appspot.com/316080043/diff/60001/src/internet/model/ipv6-q...
src/internet/model/ipv6-queue-disc-item.cc:40: {
NS_LOG_FUNCTION

same suggestion as before.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/net-dev...
File src/network/utils/net-device-queue-interface.cc (right):

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/net-dev...
src/network/utils/net-device-queue-interface.cc:74: {
Indentation

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/net-dev...
src/network/utils/net-device-queue-interface.cc:118: {
Indentation

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
File src/network/utils/queue-item.cc (right):

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:43: {
NS_LOG_FUNCTION here?

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:56: {
NS_LOG_FUNCTION here?

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:89: {
NS_LOG_FUNCTION here?

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:95: {
NS_LOG_FUNCTION here?

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:101: {
NS_LOG_FUNCTION here?

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:107: {
NS_LOG_FUNCTION here?

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.cc
File src/network/utils/queue.cc (right):

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.c...
src/network/utils/queue.cc:92: {
Should we add NS_LOG_FUNCTION here?

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.h
File src/network/utils/queue.h (right):

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.h...
src/network/utils/queue.h:251: QueueMode m_mode;                   //!< queue
mode (packets or bytes limited)
Is "limited" required?

[Stefano Avallone, 2017-03-08 15:37:06]

Thank you very much for your review. I have addressed all of your comments in
the last version of the patch I am going to push to ns-3-dev.

https://codereview.appspot.com/316080043/diff/60001/src/csma/model/csma-net-d...
File src/csma/model/csma-net-device.h (right):

https://codereview.appspot.com/316080043/diff/60001/src/csma/model/csma-net-d...
src/csma/model/csma-net-device.h:131: * DropTail or RED.
On 2017/02/26 15:22:38, Mohit P. Tahiliani wrote:
> I think documentation should be updated here.

Done.

https://codereview.appspot.com/316080043/diff/60001/src/flow-monitor/model/ip...
File src/flow-monitor/model/ipv4-flow-probe.h (right):

https://codereview.appspot.com/316080043/diff/60001/src/flow-monitor/model/ip...
src/flow-monitor/model/ipv4-flow-probe.h:116: /// \param item queue item
On 2017/02/26 15:22:38, Mohit P. Tahiliani wrote:
> \param item queue disc item

Done.

https://codereview.appspot.com/316080043/diff/60001/src/flow-monitor/model/ip...
File src/flow-monitor/model/ipv6-flow-probe.h (right):

https://codereview.appspot.com/316080043/diff/60001/src/flow-monitor/model/ip...
src/flow-monitor/model/ipv6-flow-probe.h:122: /// \param item queue item
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> \param item queue disc item

Done.

https://codereview.appspot.com/316080043/diff/60001/src/internet/model/ipv4-q...
File src/internet/model/ipv4-queue-disc-item.cc (right):

https://codereview.appspot.com/316080043/diff/60001/src/internet/model/ipv4-q...
src/internet/model/ipv4-queue-disc-item.cc:40: {
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> Should we add NS_LOG_FUNCTION here? If it was intentionally left out, then we
> can leave as it is.

Done.

https://codereview.appspot.com/316080043/diff/60001/src/internet/model/ipv6-q...
File src/internet/model/ipv6-queue-disc-item.cc (right):

https://codereview.appspot.com/316080043/diff/60001/src/internet/model/ipv6-q...
src/internet/model/ipv6-queue-disc-item.cc:40: {
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> NS_LOG_FUNCTION
> 
> same suggestion as before.

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/net-dev...
File src/network/utils/net-device-queue-interface.cc (right):

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/net-dev...
src/network/utils/net-device-queue-interface.cc:74: {
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> Indentation

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/net-dev...
src/network/utils/net-device-queue-interface.cc:118: {
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> Indentation

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
File src/network/utils/queue-item.cc (right):

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:43: {
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> NS_LOG_FUNCTION here?

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:56: {
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> NS_LOG_FUNCTION here?

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:89: {
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> NS_LOG_FUNCTION here?

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:95: {
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> NS_LOG_FUNCTION here?

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:101: {
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> NS_LOG_FUNCTION here?

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue-i...
src/network/utils/queue-item.cc:107: {
On 2017/02/26 15:22:39, Mohit P. Tahiliani wrote:
> NS_LOG_FUNCTION here?

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.cc
File src/network/utils/queue.cc (right):

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.c...
src/network/utils/queue.cc:92: {
On 2017/02/26 15:22:40, Mohit P. Tahiliani wrote:
> Should we add NS_LOG_FUNCTION here?

Uhm, it's a static method, I am not sure it makes sense to add NS_LOG_FUNCTION
here (it's not called on a QueueBase object)

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.h
File src/network/utils/queue.h (right):

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.h...
src/network/utils/queue.h:66: * \param itemType the item type
On 2017/02/22 17:29:13, Tom Henderson wrote:
> consider to add an example usage around \code ... \endcode since this method
is
> a bit unconventional

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.h...
src/network/utils/queue.h:251: QueueMode m_mode;                   //!< queue
mode (packets or bytes limited)
On 2017/02/26 15:22:40, Mohit P. Tahiliani wrote:
> Is "limited" required?

Done.

https://codereview.appspot.com/316080043/diff/60001/src/network/utils/queue.h...
src/network/utils/queue.h:279: * implementation of the class Queue methods.
On 2017/02/22 17:29:13, Tom Henderson wrote:
> perhaps even more explicit, add "(i.e., do not include queue.h in your .h
files
> but instead use the forward declaration, and then include queue.h in your .cc
> files)"

Done.