nest-simulator icon indicating copy to clipboard operation
nest-simulator copied to clipboard

Clean up comments in nestkernel

Open jessica-mitchell opened this issue 3 years ago • 5 comments

This PR aims to make the comments (doxygen and c++) more consistent and useful. So far, only a few files have been touched because I would like some feedback as to whether I'm going in the right direction before proceeding. @jougs could you take a look?

Changes implemented:

  • C++ comments single line (max 2 lines if sentence is long): //
  • C++ comments multile line: /* * * * */
  • doxygen comments single line (max 2 lines if sentence is long): //!
  • doxygen comments multiline: /** * * * * */
  • doxygen comments side: //!<
  • avoid doxygen comments in .cpp files, move to header file
  • avoid duplicating code in comments; only include code(parameters, functions etc) that also have additional context, remove redundancies
  • include the variable name in functions in header file to match cpp file.

jessica-mitchell avatar Jul 23 '21 09:07 jessica-mitchell

Thanks for making the chaos and invonsistencies visible. I'm reviewing in greater detail once the crying stops. For now, I have a couple of generic comments:

* I prefer the following doxygen style for functions and classes, even if the comment itself is only a single line:
  ```
  /**
   * ...
   */
  ```

Sure, I think you're right that it's more common

* I would use `//!` for long one-liners above variables and `//!<` for short ones behind them

When you say above variables, can you give me an example of the syntax you'd expect this kind of comment? Where would not want this comment style? (it's just easier if I have an example to look up, because it's easy to get confused staring at slashes and askterisks all day ;) )

Maybe we should set up a video session where we go through the Doxygen documentation and ask people what they (dis)like most?

Yah sure, I think that could work.

jessica-mitchell avatar Jul 25 '21 11:07 jessica-mitchell

Here are some examples that hopefully clarify my style preferences mentioned above:

/**
 * Set the minimal and maximal delay and override automatically determined values.
 */
void set_delay_extrema(long min_delay, long max_delay);

//! The variable min_delay holds the value for the smallest transmission delay in the network in steps
long min_delay_;

long max_delay_; //!< The largest transmission delay in the network (steps)

In real code, I would then actually prefer if similar variables were documented consistently, i.e. both min_delay_ and max_delay_ with either the long version, or with the short one, depending on the length of the comment. Above, I would likely prefer the short one, as the long description is a bit redundant and the short one would allow to skim through the code quicker (due to less optical clutter), cf.

//! The variable min_delay holds the value for the smallest transmission delay in the network in steps
long min_delay_;
//! The variable max_delay holds the value for the largest transmission delay in the network in steps
long max_delay_;
long min_delay_;   //!< The smallest transmission delay in the network (steps)
long max_delay_;   //!< The largest transmission delay in the network (steps)

jougs avatar Jul 26 '21 09:07 jougs

Pull request automatically marked stale!

github-actions[bot] avatar Oct 24 '21 08:10 github-actions[bot]

Pull request automatically marked stale!

github-actions[bot] avatar Jun 21 '22 08:06 github-actions[bot]

@jessica-mitchell, do you have a status update on this one?

terhorstd avatar Aug 02 '22 08:08 terhorstd

@jessica-mitchell: What is the status of this? Thanks!

jougs avatar Oct 10 '22 09:10 jougs

@jougs @heplesser I think I addressed all the comments so far. In addition to removing the Constructor comments, I also removed Destructor.

I cleaned up the guidelines (doc/htmldoc/developer_space/cppcoments.rst) to be more clear, but I am unsure of how to describe when to use C++ comment style /* */ versus doxygen style /** */.

jessica-mitchell avatar Oct 17 '22 08:10 jessica-mitchell

Pull request automatically marked stale!

github-actions[bot] avatar May 02 '23 08:05 github-actions[bot]

@jougs the conflict is fixed

jessica-mitchell avatar May 26 '23 09:05 jessica-mitchell

@jougs ping!

jessica-mitchell avatar Jul 04 '23 10:07 jessica-mitchell

@jessica-mitchell: pong!

jougs avatar Jul 04 '23 15:07 jougs