bug 557

[Mathieu Lacage, 2009-05-08 06:47:31]

Overall looks good:
1) coding style:
if ()
{
}
should be instead:
if ()
  {
    //stuff
  }
2) I am not convinced you need to pass an argument to NotifyNewAggregate:
probably easier to not pass any
3) NotifyNewAggregate implementations must do an upcall before returning
4) declrations in header files of NotifyNewAggregate must be protected (not
public) and they should repeat the virtual keyword

http://codereview.appspot.com/63048/diff/1/3
File examples/emu-ping.cc (right):

http://codereview.appspot.com/63048/diff/1/3#newcode151
Line 151: InternetStackHelper internetStackHelper;
Could you make that change and the one above in a separate patch ?

http://codereview.appspot.com/63048/diff/1/4
File src/core/object.cc (right):

http://codereview.appspot.com/63048/diff/1/4#newcode169
Line 169: currentObject->NotifyNewAggregate (currentObject->m_next);
I think that the argument to NotifyNewAggregate should be 'o' or that you should
not pass any argument to NotifyNewAggregate

http://codereview.appspot.com/63048/diff/1/7
File src/helper/internet-stack-helper.h (right):

http://codereview.appspot.com/63048/diff/1/7#newcode50
Line 50: 
needless space changes

http://codereview.appspot.com/63048/diff/1/7#newcode83
Line 83: void SetNscStack(std::string soname);
It would be nice to replace SetNscStack by SetTcp (std::string tid, std::string
name, const AttributeValue &value);

http://codereview.appspot.com/63048/diff/1/7#newcode85
Line 85: /*void SetTcp (std::string typeid,
code which is commented out or ifdefed to zero should be deleted instead

http://codereview.appspot.com/63048/diff/1/8
File src/internet-stack/arp-l3-protocol.cc (right):

http://codereview.appspot.com/63048/diff/1/8#newcode85
Line 85: Ptr<Node>node = o->GetObject<Node> ();
You could rewrite that to:
Ptr<Node> node = GetObject<Node> ();
to avoid passing around the 'o' argument to NotifyNewAggregate.

http://codereview.appspot.com/63048/diff/1/8#newcode89
Line 89: {
coding style:
if ()
  {
    //stuff
  }

http://codereview.appspot.com/63048/diff/1/8#newcode91
Line 91: }
upcall to the parent here.

http://codereview.appspot.com/63048/diff/1/10
File src/internet-stack/icmpv4-l4-protocol.cc (right):

http://codereview.appspot.com/63048/diff/1/10#newcode71
Line 71: Ptr<Ipv4RawSocketFactoryImpl> rawFactory =
CreateObject<Ipv4RawSocketFactoryImpl> ();
I feel that the raw socket factory should be aggregated by the Ipv4L3PRotocol
object instead of the icmpv4 stack.

http://codereview.appspot.com/63048/diff/1/10#newcode74
Line 74: }
Probably should do an upcall to the parent here:
Object::NotifyNewAggregate (o);

http://codereview.appspot.com/63048/diff/1/11
File src/internet-stack/icmpv4-l4-protocol.h (right):

http://codereview.appspot.com/63048/diff/1/11#newcode27
Line 27: void NotifyNewAggregate (Ptr<Object> o);
This should be a protected member. We also usually repeat the keyword 'virtual'
in subclasses to remind the reader that this is an overriden method.

http://codereview.appspot.com/63048/diff/1/14
File src/internet-stack/ipv4-l3-protocol.cc (right):

http://codereview.appspot.com/63048/diff/1/14#newcode163
Line 163: }
upcall to parent.

http://codereview.appspot.com/63048/diff/1/15
File src/internet-stack/ipv4-l3-protocol.h (right):

http://codereview.appspot.com/63048/diff/1/15#newcode70
Line 70: void NotifyNewAggregate(Ptr<Object> o);
protected virtual member. Not public.

http://codereview.appspot.com/63048/diff/1/16
File src/internet-stack/nsc-tcp-l4-protocol.cc (right):

http://codereview.appspot.com/63048/diff/1/16#newcode31
Line 31: //#include "type-id.h"
comment should be killed

http://codereview.appspot.com/63048/diff/1/16#newcode44
Line 44: 
needless space change

http://codereview.appspot.com/63048/diff/1/16#newcode187
Line 187: }
upcall to parent

http://codereview.appspot.com/63048/diff/1/17
File src/internet-stack/nsc-tcp-l4-protocol.h (right):

http://codereview.appspot.com/63048/diff/1/17#newcode57
Line 57: void NotifyNewAggregate (Ptr<Object> o);
protected virtual

http://codereview.appspot.com/63048/diff/1/17#newcode58
Line 58: void SetNscLibrary(const std::string &lib);
GetNscLibraryStr->GetNscLibrary
move code from SetNscLibraryStr to SetNscLibrary

http://codereview.appspot.com/63048/diff/1/18
File src/internet-stack/tcp-l4-protocol.cc (right):

http://codereview.appspot.com/63048/diff/1/18#newcode388
Line 388: }
upcall parent

http://codereview.appspot.com/63048/diff/1/19
File src/internet-stack/tcp-l4-protocol.h (right):

http://codereview.appspot.com/63048/diff/1/19#newcode69
Line 69: void NotifyNewAggregate (Ptr<Object> o);
protected virtual

http://codereview.appspot.com/63048/diff/1/22
File src/internet-stack/udp-l4-protocol.cc (right):

http://codereview.appspot.com/63048/diff/1/22#newcode99
Line 99: node->AggregateObject (udpFactory);
upcall parent

http://codereview.appspot.com/63048/diff/1/23
File src/internet-stack/udp-l4-protocol.h (right):

http://codereview.appspot.com/63048/diff/1/23#newcode53
Line 53: void NotifyNewAggregate (Ptr<Object> o);
protected virtual