Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(945)

Issue 4426070: Bug 1118: Implement point-to-point tree helper

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 11 months ago by John Abraham
Modified:
12 years, 11 months ago
Reviewers:
Josh Pelkey
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Patch Set 1 #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+847 lines, -14 lines) Patch
A src/netanim/examples/tree-animation.cc View 1 chunk +153 lines, -0 lines 0 comments Download
M src/netanim/examples/wscript View 1 chunk +4 lines, -0 lines 0 comments Download
M src/point-to-point-layout/model/point-to-point-star.h View 8 chunks +58 lines, -10 lines 0 comments Download
M src/point-to-point-layout/model/point-to-point-star.cc View 5 chunks +46 lines, -4 lines 1 comment Download
A src/point-to-point-layout/model/point-to-point-tree.h View 1 chunk +211 lines, -0 lines 10 comments Download
A src/point-to-point-layout/model/point-to-point-tree.cc View 1 chunk +373 lines, -0 lines 1 comment Download
M src/point-to-point-layout/wscript View 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 1
Josh Pelkey
12 years, 11 months ago (2011-04-29 15:30:57 UTC) #1
I didn't run this, but I'm sure you have tested it. Speaking of tests, I might
recommend a test case that creates a bunch of random trees and makes sure
everything goes ok. Finally, when you are done with the coding, you should run
checkstyle.py on your changes
(http://www.nsnam.org/doxygen/group___check_style.html). I noticed several
places where indentation was wrong, and spacing was weird. You might want to
check your settings in whatever editor you are using
(http://www.nsnam.org/wiki/index.php/Developer_FAQ#Coding_style)

Some initial high level inline comments below. I'd be interested to see this run
through net-anim and pyviz. Seems like it could be cool.

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
File src/point-to-point-layout/model/point-to-point-star.cc (right):

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-star.cc:196:
spokeLoc->SetPosition (spokeVec);
rogue change?

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
File src/point-to-point-layout/model/point-to-point-tree.cc (right):

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.cc:71: NS_LOG_WARN ("Number
of levels:"<<num_levels<<"<=0\nNo Topology created");
should you do more than just warn here? (assert?)

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
File src/point-to-point-layout/model/point-to-point-tree.h (right):

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.h:49: using namespace std;
we try to avoid this in ns-3, though you'll find it in some places

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.h:66: * \param num_levels
the number of levels in the tree.Root node is level 0
space after period

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.h:80: * tree topologies
using p2p links.This constructor needs to be used if the
space after period

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.h:83: * \param num_levels
the number of levels in the tree.Root node is level 0
space after period

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.h:121: * \param level Level
of the requested Node [Root node is at level 0.i.e,Tree levels are zero-indexed]
space :)

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.h:123: * \param index Index
of the requested Node [First node at a level has the index 0. i.e, The nodes at
a level are zero-indexed
do you mean left most node?

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.h:125: * \returns a node
pointer to the requested node.NULL is returned if the Node does not exist
and another space needed

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.h:138: * \return type ID
i think this should be "returns"

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.h:193: *
returns?

http://codereview.appspot.com/4426070/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-tree.h:206: vector
<NodeContainer> PerLevelNodeContainer; //each tree level maintains a container
of nodes at that level
recommend m_perLevelNodeContainerVector or something similar
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b