Re: [Ns-commits] ns-3-dev

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

Re: [Ns-commits] ns-3-dev

Barnes, Peter D.
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


Reply | Threaded
Open this post in threaded view
|

Re: [Ns-commits] ns-3-dev

Tom Henderson-2
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