[Ns-developers] Packet tag questions
mathieu.lacage at gmail.com
Fri Jan 27 01:31:42 PST 2012
On Tue, Jan 24, 2012 at 23:04, Barnes, Peter D. <barnes26 at llnl.gov> wrote:
I have very carefully reviewed the output of hg log -r c3013f70c6a3 -p
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
- 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.
- 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;
- 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 Lacage <mathieu.lacage at gmail.com>
More information about the Ns-developers