[Ns-developers] Packet tag questions

[Mathieu Lacage]

On Tue, Jan 24, 2012 at 23:04, Barnes, Peter D. <barnes26 at llnl.gov> wrote:
> http://code.nsnam.org/pdbarnes/packet-tag-list/

I have very carefully reviewed the output of hg log -r c3013f70c6a3 -p
src/network/model/packet-tag-list.h
src/network/model/packet-tag-list.cc

I trust your good taste with regard to the other changes in this repo
since I do not have time to review them.

1) generic comments:
  - you ran the regression tests, I presume. good :)
  - NS_LOG_FUNCTION_NOARGS sucks. Use NS_LOG_FUNCTION(this <<) instead
whenever you can.
  - In general, I like to avoid using typedefs to hide struct
definitions (similar statements by Linus tend to weigh more than my
lengthy explanations: http://yarchive.net/comp/linux/typedefs.html). I
suspect that you used it mostly to make doxygen happy. I can live with
it but I wanted to point out that I feel that it is best avoided when
possible.
  - There is a reason why the last line was written as such: older
versions of g++ were not happy with the use of a C++ comment on the
same line as an #endif. Do not ask why.
-#endif /* PACKET_TAG_LIST_H */
+#endif // PACKET_TAG_LIST_H
  - a lot of space changes to the code. This really sucks. Why so ?
Did you run it through the style checker ?
  - do we really use the PacketTagList::Head method somewhere ? If not, kill it.

2) details
  - the memset in +PacketTagList::FreeData should not be needed from a
functional point of view. It is merely a debug helper I think. If so,
#ifdef _DEBUG would be better from a performance point of view and
setting everything to 0x66 instead of 0x00 is likely to be much more
useful in spotting garbage early
  - The free list is disabled by default, right ? If so, did you try
to benchmark whether or not it makes sense to enable it ? If it makes
no sense, I would say, kill the #ifdef.
  - There is a good reason for that kind of code: it makes it easier
in a debugger to know what the return value is.
-  struct PacketTagList::TagData *retval;
-  retval = new struct PacketTagList::TagData ();
-  return retval;

3) implementation
  - I think that the code did not really use the count field before
this patch. I thought that I used it to perform the optimization that
you implemented but I have been unable to find any piece of code in my
archives that implemented this. Sounds like I planned to do this
someday but forgot about it and never did. It's good that you did.
  - did you benchmark the new implementation ? Does it improve the
runtime of actual macro benchmarks ?

That's it. +1 for merging.

Mathieu
-- 
Mathieu Lacage <mathieu.lacage at gmail.com>