Re: ns-3 coding style (issue165087)

[tomh...@gmail.com]

Reviewers: Andrey Mazo, gjcarneiro,

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.



Please review this at http://codereview.appspot.com/165087

Affected files:
M html_src/code-submission.html
M html_src/codingstyle.html

[mathieu...@gmail.com]

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");

http://codereview.appspot.com/165087

[anh...@gmail.com]

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.

http://codereview.appspot.com/165087

[Mathieu Lacage]

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...@gmail.com>

[anh...@gmail.com]

> > 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.

https://codereview.appspot.com/165087

[tomh...@gmail.com]

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.

http://codereview.appspot.com/165087