Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(463)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 6349088: GSoC - 2012 Mid Term Code Review (HLA interfaces for ns-3)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 11 months ago by mudit
Modified:
13 years, 11 months ago
Reviewers:
aquereilhac
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

GSoC - 2012 Mid Term Code Review (HLA interfaces for ns-3)

Patch Set 1 #

Total comments: 17
Unified diffs Side-by-side diffs Delta from patch set Stats (+3729 lines, -0 lines) Patch
A src/hla/RTI/scripts/setVar.sh View 1 chunk +14 lines, -0 lines 0 comments Download
A src/hla/RTI/scripts/tst.cc View 1 chunk +11 lines, -0 lines 0 comments Download
A src/hla/doc/hla View 1 chunk +135 lines, -0 lines 3 comments Download
A src/hla/examples/example-plus-rti-listen.cc View 1 chunk +142 lines, -0 lines 2 comments Download
A src/hla/model/LinkToRti/link_to_rti.h View 1 chunk +72 lines, -0 lines 1 comment Download
A src/hla/model/LinkToRti/link_to_rti.cc View 1 chunk +168 lines, -0 lines 1 comment Download
A src/hla/model/ns3Federate/linux.sh View 1 chunk +77 lines, -0 lines 0 comments Download
A src/hla/model/ns3Federate/ns3Federate.jar View Binary file 0 comments Download
A src/hla/model/ns3Federate/src/ns3FedAmb.java View 1 chunk +331 lines, -0 lines 1 comment Download
A src/hla/model/ns3Federate/src/ns3Federate.java View 1 chunk +362 lines, -0 lines 0 comments Download
A src/hla/model/ns3Federate/testfom.fed View 1 chunk +50 lines, -0 lines 0 comments Download
A src/hla/test/TestMessages.cc View 1 chunk +68 lines, -0 lines 1 comment Download
A src/hla/test/TestMessages.cc~ View 1 chunk +72 lines, -0 lines 1 comment Download
A src/hla/test/dummy/dummy.jar View Binary file 0 comments Download
A src/hla/test/dummy/linux.sh View 1 chunk +76 lines, -0 lines 0 comments Download
A src/hla/test/dummy/linux.sh~ View 1 chunk +76 lines, -0 lines 1 comment Download
A src/hla/test/dummy/logs/portico.log View 1 chunk +47 lines, -0 lines 1 comment Download
A src/hla/test/dummy/src/dummy.java~ View 1 chunk +589 lines, -0 lines 1 comment Download
A src/hla/test/dummy/src/dummyFederate.java View 1 chunk +400 lines, -0 lines 0 comments Download
A src/hla/test/dummy/src/dummyFederate.java~ View 1 chunk +401 lines, -0 lines 1 comment Download
A src/hla/test/dummy/src/dummyFederateAmbassador.java View 1 chunk +248 lines, -0 lines 0 comments Download
A src/hla/test/dummy/src/dummyFederateAmbassador.java~ View 1 chunk +248 lines, -0 lines 1 comment Download
A src/hla/test/dummy/testfom.fed View 1 chunk +50 lines, -0 lines 0 comments Download
A src/hla/test/dummy/testfom.fed~ View 1 chunk +50 lines, -0 lines 1 comment Download
A src/hla/test/ex.cc~ View 1 chunk +8 lines, -0 lines 1 comment Download
A src/hla/test/rawTest.cc View 1 chunk +11 lines, -0 lines 0 comments Download
A src/hla/waf View 1 chunk +1 line, -0 lines 0 comments Download
A src/hla/wscript View 1 chunk +22 lines, -0 lines 0 comments Download

Messages

Total messages: 2
aquereilhac
Dear all, Please find a code review for the HLA GSoC12 project by Mudit Raj ...
13 years, 11 months ago (2012-07-16 10:39:26 UTC) #1
mudit
13 years, 11 months ago (2012-07-16 14:18:30 UTC) #2 
Hi Alina,

Thank you for your review, it's certainly very useful. I will surely apply
the coding style to all the documents and remove the unwanted *.java~ files
and try to address the comments one by one.

Thank you for your time.

Best Regards,

Mudit Raj Gupta



On Mon, Jul 16, 2012 at 4:09 PM, <aquereilhac@gmail.com> wrote:

> Dear all,
>
> Please find a code review for the HLA GSoC12 project by Mudit Raj Gupta.
>
> I find the document explaining the 'Approach' of the project [1] clear
> and well written.
>
> Also, I strongly recommend to the author to apply the coding style, as
> defined in [2], to the code.
>
> Best regards,
>
> Alina.
>
> [1]
> http://www.google-melange.com/**gsoc/proposal/review/google/**
>
gsoc2012/muditrajgupta/45002<http://www.google-melange.com/gsoc/proposal/review/google/gsoc2012/muditrajgupta/45002>
>
> [2]
http://www.nsnam.org/**developers/contributing-code/**coding-style/<http://ww...
>
>
>
>
http://codereview.appspot.com/**6349088/diff/1/src/hla/doc/hla<http://coderev...
> File src/hla/doc/hla (right):
>
>
http://codereview.appspot.com/**6349088/diff/1/src/hla/doc/**hla#newcode52<ht...
> src/hla/doc/hla:52: It takes care of the creating a federating (if not
> created else joining it), registering
> Meaning of this sentence not so clear -> "It takes care of creating a
> 'federate'"?
>
>
http://codereview.appspot.com/**6349088/diff/1/src/hla/doc/**hla#newcode54<ht...
> src/hla/doc/hla:54: and resistering objects and interaction in the RTI.
> registering?
>
>
http://codereview.appspot.com/**6349088/diff/1/src/hla/doc/**hla#newcode96<ht...
> src/hla/doc/hla:96: can be easily by adding similar functions as objects
> can be easily 'added'?
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/**
>
examples/example-plus-rti-**listen.cc<http://codereview.appspot.com/6349088/diff/1/src/hla/examples/example-plus-rti-listen.cc>
> File src/hla/examples/example-plus-**rti-listen.cc (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/**
>
examples/example-plus-rti-**listen.cc#newcode36<http://codereview.appspot.com/6349088/diff/1/src/hla/examples/example-plus-rti-listen.cc#newcode36>
> src/hla/examples/example-plus-**rti-listen.cc:36: readFed();
> Although declaring a function signature before the main and the
> implementation after is correct, usually in ns-3 example programs
> functions are directly implemented before the main.
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/**
>
examples/example-plus-rti-**listen.cc#newcode90<http://codereview.appspot.com/6349088/diff/1/src/hla/examples/example-plus-rti-listen.cc#newcode90>
> src/hla/examples/example-plus-**rti-listen.cc:90: void readFed()
> Please take a look at the code layout on the ns-3 coding style
> guidelines [1].
>
> [1]
> http://www.nsnam.org/**developers/contributing-code/**
>
coding-style/#code-layout<http://www.nsnam.org/developers/contributing-code/coding-style/#code-layout>
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/model/**
>
LinkToRti/link_to_rti.cc<http://codereview.appspot.com/6349088/diff/1/src/hla/model/LinkToRti/link_to_rti.cc>
> File src/hla/model/LinkToRti/link_**to_rti.cc (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/model/**
>
LinkToRti/link_to_rti.cc#**newcode35<http://codereview.appspot.com/6349088/diff/1/src/hla/model/LinkToRti/link_to_rti.cc#newcode35>
> src/hla/model/LinkToRti/link_**to_rti.cc:35: //socket related variables
> Is there a good reason why this scope was chosen for these variables
> instead of making them class members?
> If there is no special reason for this I would recommend making them
> class members.
>
> Also, ns-3 coding style guidelines are not respected for variables,
> functions, and class names. Please refer to
>
http://www.nsnam.org/**developers/contributing-code/**coding-style/<http://ww....
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/model/**
>
LinkToRti/link_to_rti.h<http://codereview.appspot.com/6349088/diff/1/src/hla/model/LinkToRti/link_to_rti.h>
> File src/hla/model/LinkToRti/link_**to_rti.h (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/model/**
>
LinkToRti/link_to_rti.h#**newcode37<http://codereview.appspot.com/6349088/diff/1/src/hla/model/LinkToRti/link_to_rti.h#newcode37>
> src/hla/model/LinkToRti/link_**to_rti.h:37: class link_to_rti
> ns-3 coding style guidelines are not respected for variables, functions,
> and class names. Please refer to
>
http://www.nsnam.org/**developers/contributing-code/**coding-style/<http://ww....
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/model/**
>
ns3Federate/src/ns3FedAmb.java<http://codereview.appspot.com/6349088/diff/1/src/hla/model/ns3Federate/src/ns3FedAmb.java>
> File src/hla/model/ns3Federate/src/**ns3FedAmb.java (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/model/**
>
ns3Federate/src/ns3FedAmb.**java#newcode45<http://codereview.appspot.com/6349088/diff/1/src/hla/model/ns3Federate/src/ns3FedAmb.java#newcode45>
> src/hla/model/ns3Federate/src/**ns3FedAmb.java:45:
> Although I am not sure there if there is a code style for java classes
> in ns-3, a same style should be used always.
> In this class some methods use CamelCase and others camelBack.
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
TestMessages.cc<http://codereview.appspot.com/6349088/diff/1/src/hla/test/TestMessages.cc>
> File src/hla/test/TestMessages.cc (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
TestMessages.cc#newcode10<http://codereview.appspot.com/6349088/diff/1/src/hla/test/TestMessages.cc#newcode10>
> src/hla/test/TestMessages.cc:**10:
> Consider using TestCase and TestSuite classes for ns-3 C++ tests. As an
> example, take a look at src/point-to-point/test/point-**to-point-test.cc .
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
TestMessages.cc~<http://codereview.appspot.com/6349088/diff/1/src/hla/test/TestMessages.cc%7E>
> File src/hla/test/TestMessages.cc~ (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
TestMessages.cc~#newcode8<http://codereview.appspot.com/6349088/diff/1/src/hla/test/TestMessages.cc%7E#newcode8>
> src/hla/test/TestMessages.cc~:**8:
> All ".cc~" files should be removed from the patch
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
dummy/linux.sh~<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/linux.sh%7E>
> File src/hla/test/dummy/linux.sh~ (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
dummy/linux.sh~#newcode2<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/linux.sh%7E#newcode2>
> src/hla/test/dummy/linux.sh~:**2:
> All ".sh~" files should be removed from the patch
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
dummy/logs/portico.log<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/logs/portico.log>
> File src/hla/test/dummy/logs/**portico.log (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
dummy/logs/portico.log#**newcode1<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/logs/portico.log#newcode1>
> src/hla/test/dummy/logs/**portico.log:1: ERROR [main] portico.lrc:
> federation exists: ExampleFederation
> Should this file be part of the patch?
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
dummy/src/dummy.java~<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/src/dummy.java%7E>
> File src/hla/test/dummy/src/dummy.**java~ (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
dummy/src/dummy.java~#newcode1<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/src/dummy.java%7E#newcode1>
> src/hla/test/dummy/src/dummy.**java~:1: /*
> All ".java~" files should be removed from the patch
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
dummy/src/dummyFederate.java~<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/src/dummyFederate.java%7E>
> File src/hla/test/dummy/src/**dummyFederate.java~ (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
dummy/src/dummyFederate.java~#**newcode2<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/src/dummyFederate.java%7E#newcode2>
> src/hla/test/dummy/src/**dummyFederate.java~:2: /*
> All ".java~" files should be removed from the patch
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**dummy/src/**
>
dummyFederateAmbassador.java~<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/src/dummyFederateAmbassador.java%7E>
> File src/hla/test/dummy/src/**dummyFederateAmbassador.java~ (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**dummy/src/**
>
dummyFederateAmbassador.java~#**newcode2<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/src/dummyFederateAmbassador.java%7E#newcode2>
> src/hla/test/dummy/src/**dummyFederateAmbassador.java~:**2: /*
> All ".java~" files should be removed from the patch
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
dummy/testfom.fed~<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/testfom.fed%7E>
> File src/hla/test/dummy/testfom.**fed~ (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
dummy/testfom.fed~#newcode1<http://codereview.appspot.com/6349088/diff/1/src/hla/test/dummy/testfom.fed%7E#newcode1>
> src/hla/test/dummy/testfom.**fed~:1: ;; A comment in the test file, just
> to show I'm cool ;;
> All ".fed~" files should be removed from the patch
>
>
http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**ex.cc~<http://c...
> File src/hla/test/ex.cc~ (right):
>
> http://codereview.appspot.com/**6349088/diff/1/src/hla/test/**
>
ex.cc~#newcode2<http://codereview.appspot.com/6349088/diff/1/src/hla/test/ex.cc%7E#newcode2>
> src/hla/test/ex.cc~:2:
> All ".cc~" files should be removed from the patch
>
>
http://codereview.appspot.com/**6349088/<http://codereview.appspot.com/6349088/>
>
Sign in to reply to this message.

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