Re: Ns-developers Digest, Vol 123, Issue 14

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Ns-developers Digest, Vol 123, Issue 14

Ammon, Robert (ammo6818@vandals.uidaho.edu)
I completely understand Peter's comment.  In making the corrections, I have attempted to provide as much detail as I could in a reasonable time frame.  As Peter has noted, some of the changes are quite minimal when it comes to detail.


As Tom has noted, the maintainers that have processed changes for their  modules have reviewed the changes provided and updated the ones that they felt needed more details.  The maintainers that have already processed the changes have made the updates themselves.


In addition to Peter's example, there are lots of simple corrections, /** instead of /*, /// instead of //, variable name mismatches, /param and /return instead of \param  and \return, etc.


If any maintainer feels that any of the changes are insufficient and does not want to take on the workload of updating the changes to add more detail, I would be willing to revisit them and attempt to add additional details to satisfy their concerns.  IMHO the only practical way to do this would be for the maintainers to process the existing changes and merge the changes that are acceptable into the master.  Once that happens, I would be willing to take another pass through the files and update the remainder warnings.  I understand that means multiple merges into the master, but I believe its the best process for maintainers that having concerns.


As I said, I agree with Peter's and Tom's comments about sufficient detail.  However, Bug 938 for doxygen warnings has been open and in progress for almost 7 years now.  As I have previously written, the biggest reason that this has not been completely resolved is the process the project is using does not prevent new warnings from being introduced.  For example, the latest changes to LTE and WIFI introduced several hundred new warnings.  The only way to completely resolve this issue is to change the master build options to fail if there are any warnings so they will be addressed right away and the only way to achieve that is to get every module in the project to 0 doxygen warnings.


While strong comments are everyone's goal  and the best comment possible should be attained, holding changes open for better comments has the negative effect on the project of allowing additional warnings to be introduced.


If the maintainers will provide guidance on what they need me to do going forward, I will make the efforts required to achieve closure of this issue.


________________________________
From: [hidden email] <[hidden email]> on behalf of [hidden email] <[hidden email]>
Sent: Friday, February 24, 2017 6:00:02 PM
To: [hidden email]
Subject: Ns-developers Digest, Vol 123, Issue 14

Send Ns-developers mailing list submissions to
        [hidden email]

To subscribe or unsubscribe via the World Wide Web, visit
        http://mailman.isi.edu/mailman/listinfo/ns-developers
or, via email, send a message with subject or body 'help' to
        [hidden email]

You can reach the person managing the list at
        [hidden email]

When replying, please edit your Subject line so it is more specific
than "Re: Contents of Ns-developers digest..."


Today's Topics:

   1. Re: [Ns-commits] ns-3-dev (Barnes, Peter D.)
   2. Re: [Ns-commits] ns-3-dev (Tom Henderson)


----------------------------------------------------------------------

Message: 1
Date: Fri, 24 Feb 2017 18:15:42 +0000
From: "Barnes, Peter D." <[hidden email]>
Subject: Re: [Ns-developers] [Ns-commits] ns-3-dev
To: "Ammon, Robert ([hidden email])"
        <[hidden email]>
Cc: ns-developers list <[hidden email]>
Message-ID: <[hidden email]>
Content-Type: text/plain; charset="utf-8"

Hello Robert,

Thank you for your monumental work in proposing complete doxygen for all that?s missing.  However, I have to say this commit is really inadequate, for the reasons that have been discussed previously on the list and in the bug report comments.

For example,

/// BenchHeader class
template <int N>
class BenchHeader : public Header

Well, sure, this is the BenchHeader class.  But what does it *do*?  How is it used?  What does the template argument mean?

  /**
   * Is OK function
   * \returns true if OK
   */
  bool IsOk (void) const;
  ...
  bool m_ok; ///< is OK

What constitutes ?ok??

Of course I can read the code and find out, but that just underlines the point:  this kind of documentation isn?t helpful.  Documentation should explain the intent, available functionality: the *what*.  I should only have to read the code if I want to understand the implementation, the *how*.

I strongly encourage us all to strive first for useful, if not complete, documentation, rather than the reverse.

Peter

> On Feb 24, 2017, at 9:22 AM, [hidden email] wrote:
>
> ---- utils:  Doxygen updates for utils directory
> user: Robert Ammon <[hidden email]>
> files: utils/bench-packets.cc utils/bench-simulator.cc utils/check-style.py utils/grid.py utils/print-introspected-doxygen.cc utils/python-unit-tests.py utils/tests/TestBase.py
> url: http://code.nsnam.org/ns-3-dev/rev/6f23ea9219d6
>
> _______________________________________________

_______________________________________________________________________
Dr. Peter D. Barnes, Jr.                NACS Division
Lawrence Livermore National Laboratory  Physical and Life Sciences
7000 East Avenue, L-50                  email:  [hidden email]
P. O. Box 808                           Voice:  (925) 422-3384
Livermore, California 94550             Fax:    (925) 423-3371




------------------------------

Message: 2
Date: Fri, 24 Feb 2017 10:50:02 -0800
From: Tom Henderson <[hidden email]>
Subject: Re: [Ns-developers] [Ns-commits] ns-3-dev
To: "Barnes, Peter D." <[hidden email]>, "Ammon,     Robert
        ([hidden email])" <[hidden email]>
Cc: ns-developers list <[hidden email]>
Message-ID: <[hidden email]>
Content-Type: text/plain; charset=utf-8; format=flowed

On 02/24/2017 10:15 AM, Barnes, Peter D. wrote:

> Hello Robert,
>
> Thank you for your monumental work in proposing complete doxygen for all that?s missing.  However, I have to say this commit is really inadequate, for the reasons that have been discussed previously on the list and in the bug report comments.
>
> For example,
>
> /// BenchHeader class
> template <int N>
> class BenchHeader : public Header
>
> Well, sure, this is the BenchHeader class.  But what does it *do*?  How is it used?  What does the template argument mean?
>
>    /**
>     * Is OK function
>     * \returns true if OK
>     */
>    bool IsOk (void) const;
>    ...
>    bool m_ok; ///< is OK
>
> What constitutes ?ok??
>
> Of course I can read the code and find out, but that just underlines the point:  this kind of documentation isn?t helpful.  Documentation should explain the intent, available functionality: the *what*.  I should only have to read the code if I want to understand the implementation, the *how*.
>
> I strongly encourage us all to strive first for useful, if not complete, documentation, rather than the reverse.

Hi Peter,
I will take blame for this, as I was the one who reviewed Robert's patch
and pushed it this morning.  I would like to get these patches merged
before the codebase changes further.

As for the "IsOk" method, when reviewing this morning, I had already
touched up the doxygen for that particular method with more information
about the IsOk method (basically, to say that it is a dummy method on a
dummy header class), but I see now that my edits didn't get committed.
Somehow I must have overwrote my local edits before I committed the
patch; sorry about that.

As for your broader point, I agree that we need to avoid
underdocumentation to clear warnings.  However, in this directory, some
of these warnings are for really mundane methods of utility scripts and
programs, and the patch was over 1700 lines long, so I did not spend too
much time on editing this particular patch (as much as I would on an
actual library module).

- Tom



------------------------------

_______________________________________________
Ns-developers mailing list
[hidden email]
http://mailman.isi.edu/mailman/listinfo/ns-developers


End of Ns-developers Digest, Vol 123, Issue 14
**********************************************
Reply | Threaded
Open this post in threaded view
|

Re: Ns-developers Digest, Vol 123, Issue 14

Natale Patriciello
On 25/02/17 at 04:24, Ammon, Robert ([hidden email]) wrote:
>
> If the maintainers will provide guidance on what they need me to do
> going forward, I will make the efforts required to achieve closure of
> this issue.


No directly related to the mentioned issue,


in the Linux kernel, patches are not accepted if they don't pass a
little script (checkpatch.pl) which check patch coding stlye and other
things. We can do the same, writing a script that process the patch by
checking coding style, documentation, and other issue that we feel
important.

Nat
Reply | Threaded
Open this post in threaded view
|

Re: Ns-developers Digest, Vol 123, Issue 14

Tom Henderson-2
On 02/26/2017 02:51 AM, Natale Patriciello wrote:

> On 25/02/17 at 04:24, Ammon, Robert ([hidden email]) wrote:
>> If the maintainers will provide guidance on what they need me to do
>> going forward, I will make the efforts required to achieve closure of
>> this issue.
>
> No directly related to the mentioned issue,
>
>
> in the Linux kernel, patches are not accepted if they don't pass a
> little script (checkpatch.pl) which check patch coding stlye and other
> things. We can do the same, writing a script that process the patch by
> checking coding style, documentation, and other issue that we feel
> important.
>
> Nat

We experimented with a pre-commit hook a few years ago for style
checking, and found that since our style script relies on uncrustify,
which itself produces different results across versions, that it was not
going to be workable.  I think we would need to remove the dependency to
make it work as a pre-commit hook.

Documentation takes a while to build and so I don't think we want to
block a push on that; we'd have to set up some kind of staging process.

I'm open to the idea of such a hook if someone wants to work on it, but
what I plan to change once we clear the warnings in ns-3-dev is a 'build
failure' declaration in the script that already runs the
doxygen.warnings.report.sh.  This script runs daily and outputs the
results of doxygen.warnings.report.sh but it does not currently indicate
build failure for the presence of warnings; e.g.:

http://mailman.isi.edu/pipermail/ns-commits/2017-February/019670.html

This status report could be easily changed once we clear the doxygen
warnings in ns-3-dev.

- Tom