Issue 165087: ns-3 coding style
|
Created:
16 years, 6 months ago by Tom Henderson Modified:
16 years, 6 months ago CC:
ns-3-reviews_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 21
Patch Set 2 : second round of proposed coding style changes #
MessagesTotal messages: 8
http://codereview.appspot.com/165087/diff/1/2 File html_src/code-submission.html (right): http://codereview.appspot.com/165087/diff/1/2#newcode96 html_src/code-submission.html:96: with our codebase. This is mostly a copy&paste from "General Guidelines" of codingstyle.html. Maybe is it better to just put a reference here? http://codereview.appspot.com/165087/diff/1/2#newcode114 html_src/code-submission.html:114: <p> Maintainers should be free to ask for rewrites of portions that modify "to ask for" or "to be asked for"? Sorry, if this is just my bad English. http://codereview.appspot.com/165087/diff/1/3 File html_src/codingstyle.html (right): http://codereview.appspot.com/165087/diff/1/3#newcode244 html_src/codingstyle.html:244: <p> XXX what to do about enum names? Use or avoid the trailing _e? Seems, that the existing codebase mostly avoids the trailing _e or E. Most of the names of enums contain something like Type, Mode, Class, Standard, State and thus it's generally clear, that a enum is mentioned. So +1 from me to avoid the trailing _e. http://codereview.appspot.com/165087/diff/1/3#newcode478 html_src/codingstyle.html:478: <li> Doygen grouping may be used; an example of this is the AODV module s/Doygen/Doxygen/
Sign in to reply to this message.
A note about Python style... http://codereview.appspot.com/165087/diff/1/3 File html_src/codingstyle.html (right): http://codereview.appspot.com/165087/diff/1/3#newcode34 html_src/codingstyle.html:34: <p>In general, the Python code layout follows the XXX. Python style is given by PEP 8: http://www.python.org/dev/peps/pep-0008/ Yes, it is odd that python uses func(...) while NS-3 C++ uses func (...), but PEP 8 is already a widely used standard, worth keeping...
Sign in to reply to this message.
As discussed on list, I'll also do an audit of the codebase to see whether we have predominantly Foo (void) or Foo () in function arguments. http://codereview.appspot.com/165087/diff/1/2 File html_src/code-submission.html (right): http://codereview.appspot.com/165087/diff/1/2#newcode96 html_src/code-submission.html:96: with our codebase. On 2009/12/08 10:34:01, Andrey Mazo wrote: > This is mostly a copy&paste from "General Guidelines" of codingstyle.html. > Maybe is it better to just put a reference here? OK, I will try to avoid duplication in the next revision. http://codereview.appspot.com/165087/diff/1/2#newcode114 html_src/code-submission.html:114: <p> Maintainers should be free to ask for rewrites of portions that modify On 2009/12/08 10:34:01, Andrey Mazo wrote: > "to ask for" or "to be asked for"? > Sorry, if this is just my bad English. "Maintainers should be free to ask contributors for rewrites.." http://codereview.appspot.com/165087/diff/1/3 File html_src/codingstyle.html (right): http://codereview.appspot.com/165087/diff/1/3#newcode34 html_src/codingstyle.html:34: <p>In general, the Python code layout follows the XXX. On 2009/12/08 11:23:01, gjcarneiro wrote: > Python style is given by PEP 8: http://www.python.org/dev/peps/pep-0008/ > > Yes, it is odd that python uses func(...) while NS-3 C++ uses func (...), but OK, thanks. > PEP 8 is already a widely used standard, worth keeping... > http://codereview.appspot.com/165087/diff/1/3#newcode244 html_src/codingstyle.html:244: <p> XXX what to do about enum names? Use or avoid the trailing _e? On 2009/12/08 10:34:01, Andrey Mazo wrote: > Seems, that the existing codebase mostly avoids the trailing _e or E. > Most of the names of enums contain something like Type, Mode, Class, Standard, > State and thus it's generally clear, that a enum is mentioned. > So +1 from me to avoid the trailing _e. I tend to think of us having this trailing _e (look at ArpCacheEntry). Anyway, I don't mind to remove it and it shouldn't really be a noticeable change.
Sign in to reply to this message.
hi tom, thank you for doing the work to start this discussion. Detailed comments are shown below but I have two main high-level comments: - I feel that the current document does not convey sufficiently clearly the distinction between hard vs soft requirements - we should probably include some kind of TOC outline at the top of each document to make it easier to see the big picture. http://codereview.appspot.com/165087/diff/1/2 File html_src/code-submission.html (right): http://codereview.appspot.com/165087/diff/1/2#newcode68 html_src/code-submission.html:68: <p>The basic purpose of the the code reviews is to ensure the long-term s/of the the/of the/ http://codereview.appspot.com/165087/diff/1/2#newcode74 html_src/code-submission.html:74: Some basic principles of the ns-3 reviews are: <p>Some http://codereview.appspot.com/165087/diff/1/3 File html_src/codingstyle.html (right): http://codereview.appspot.com/165087/diff/1/3#newcode24 html_src/codingstyle.html:24: will try to differentiate between hard requirements and guidelines or I scanned through the document and I have to confess that I don't think that it clearly conveys the difference between hard requirements and guidelines. I would tend to suggest splitting them in two separate sections or documents. http://codereview.appspot.com/165087/diff/1/3#newcode130 html_src/codingstyle.html:130: if (condition) statement // permitted if it fits on one line If you really must allow this, I would write it that way: if (condition) a_single_statement // permitted if it firs on one line http://codereview.appspot.com/165087/diff/1/3#newcode132 html_src/codingstyle.html:132: if (condition) { statement } // permitted if it fits on one line Same as above http://codereview.appspot.com/165087/diff/1/3#newcode166 html_src/codingstyle.html:166: varTwo; // not OK (return type should be on same line) Should that not be "type of variable should be on same time" ? http://codereview.appspot.com/165087/diff/1/3#newcode198 html_src/codingstyle.html:198: case letters, and may optionally end with a "_t". Defined constants Can't we just request that either all or no typedefs use _t ? http://codereview.appspot.com/165087/diff/1/3#newcode420 html_src/codingstyle.html:420: <p>Every function should be preceded by a detailed (Doxygen) comment block Every public function http://codereview.appspot.com/165087/diff/1/3#newcode423 html_src/codingstyle.html:423: Every class declaration should be preceded by a Every public class http://codereview.appspot.com/165087/diff/1/3#newcode597 html_src/codingstyle.html:597: NS_LOG_FUNCTION (mtu); I would like to recommend NS_LOG_FUNCTION (this << mtu); instead. We should also explicitely discourage the use of NS_LOG_FUNCTION_NO_ARGS unless for static functions http://codereview.appspot.com/165087/diff/1/3#newcode616 html_src/codingstyle.html:616: NS_FATAL_ERROR (condition) NS_FATAL_ERROR ("Some text to explain what went wrong and what you could do to fix it");
Sign in to reply to this message.
http://codereview.appspot.com/165087/diff/1/3 File html_src/codingstyle.html (right): http://codereview.appspot.com/165087/diff/1/3#newcode423 html_src/codingstyle.html:423: Every class declaration should be preceded by a On 2009/12/14 13:22:29, Mathieu Lacage wrote: > Every public class I believe, that "every class" would be better. Although this requirement is stricter, it will help people to easily dive into realization-specific peculiarities and it is not so impossible as per private function doxygen.
Sign in to reply to this message.
On Tue, Dec 15, 2009 at 7:28 PM, <anhippo@gmail.com> wrote: > > http://codereview.appspot.com/165087/diff/1/3 > File html_src/codingstyle.html (right): > > http://codereview.appspot.com/165087/diff/1/3#newcode423 > html_src/codingstyle.html:423: Every class declaration should be > preceded by a > On 2009/12/14 13:22:29, Mathieu Lacage wrote: > >> Every public class >> > I believe, that "every class" would be better. > Although this requirement is stricter, it will help people to easily > dive into realization-specific peculiarities and it is not so impossible > as per private function doxygen. It would be nice but it would be a fairly big thing to ask contributors. i.e., we don't even do this in the existing code: I feel bad to be asking others to do more work than I have done myself. Mathieu -- Mathieu Lacage <mathieu.lacage@gmail.com>
Sign in to reply to this message.
> > I believe, that "every class" would be better. > It would be nice but it would be a fairly big thing to ask contributors. > i.e., we don't even do this in the existing code: I feel bad to be asking > others to do more work than I have done myself. Well, I'm not talking about documenting every private function or writing long comprehensive comments to private classes. I'm trying to request at least some short comments (e.g. 3-line long) to each private class. I agree, that this is generally missing in the existing code. And particularly that is why I'd like to see comments to all classes. Of course, adding such comments to the current codebase is a big problem and may be interpreted as a longterm task. I won't insist too much on this issue, but I think I've made my point.
Sign in to reply to this message.
On 2009/12/14 13:22:29, Mathieu Lacage wrote: > hi tom, > > thank you for doing the work to start this discussion. Detailed comments are > shown below but I have two main high-level comments: > - I feel that the current document does not convey sufficiently clearly the > distinction between hard vs soft requirements > - we should probably include some kind of TOC outline at the top of each > document to make it easier to see the big picture. Sure, if you want to take a pass at it, go ahead, or let me know and I will make another draft.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
