On Sat, Jun 15, 2002 at 03:23:11PM +1000, Darren Reed wrote:
> 
> One of my pet hates is the lack of documentation with much of the code,
> ie. comments.  As we move forward, more and more code gets written and
> it doesn't get any easier to understand how it works.  At present we
> are without much in the way of SMP support but once that is added, the
> code will become even more complex, over time.
> 
> What I'd like to suggest is that in going forward, we adopt some changes
> to the KNF that require people to at least put more text in a comment
> block prior to the function.

I hate mandatory comment blocks before functions!
They tend to end up telling you stuff that can be figured out
immediately - rather than stuff that is slightly obscure.

> 
> Some things which I think would be or are going to be useful:
> 
> - statement about which locks are expected to be held and how.
>   this should include extra comments if a lock is acquired or released
>   prior to returning.

It is MUCH more important to add comments to structure definitions
indicating what the fields are for, and which lock covers each
field.  Then you can CHECK whether the routine is doing the right thing.

Comment blocks (esp. those above functions) get out of date very
easily.

> - statement about which interrupts are expected to be masked or not.
>   this should include extra comments if an interupt is masked or unmasked
>   prior to returning.
> 
> - statement about what the function does, including what is considered
>   to be a successful call or not.  This may or may not include talking
>   about what gets returned.

Actually sometimes a few lines at the top of a source file
would be nice!

> 
> To go through and do this retrospectively is a huge task so I am not
> suggesting that, rather as people touch various routines where they
> may or may not alter behaviour, more comments get added.  I think that
> updating the KNF is the correct way to encourage more people to do
> this.  A suggestion for pre-function comments might look like is this:
> 
> /*
>  * Function: foo
>  * Locks:    big_lock(M)
>  */
> 
> Where "M" is a mutex.  Some set of mneumonics needs to be standardised
> upon here to make the lock list both meaningful and concise.

Well that one is completely pointless!

> 
> /*
>  * Function:   foo_rx
>  * Interrupts: imp
>  */
> 
> "imp" from "splimp".

so is taht!

> 
> So where does the inspiration for this come from ?  Working on commercial
> projects and liking the result and seeing benefits in change to IPFilter.
> I'm not saying NetBSD should use the same or even anything like what I've
> been doing, but it can't hurt to make pre-function comments more expansive
> than they currently are.
> 
> An example from IPFilter:
> /* ------------------------------------------------------------------------ */
> /* Function:    nat_new                                                     */
> /* Returns:     nat_t* - NULL == failure to create new NAT structure,       */
> /*                       else pointer to new NAT structure                  */
> /* Parameters:  fin(I)       - pointer to packet information                */
> /*              np(I)        - pointer to NAT rule                          */
> /*              natsave(I)   - pointer to where to store NAT struct pointer */
> /*              flags(I)     - flags describing the current packet          */
> /*              direction(I) - direction of packet (in/out)                 */
> /* Write Lock:  ipf_nat                                                     */
> /*                                                                          */
> /* Attempts to create a new NAT entry.  Does not actually change the packet */
> /* in any way.                                                              */
> /*                                                                          */
> /* This fucntion is in three main parts: (1) deal with creating a new NAT   */
> /* structure for a "MAP" rule (outgoing NAT translation); (2) deal with     */
> /* creating a new NAT structure for a "RDR" rule (incoming NAT translation) */
> /* and (3) building that structure and putting it into the NAT table(s).    */
> /* ------------------------------------------------------------------------ */
> 
> Extremely verbose, and perhaps over verbose on the "Parameters" side.
> I'm not saying NetBSD should use that but perhaps think into the future,
> now, about what would be good to have and set a direction for others
> here to follow when hacking on NetBSD code.

Degfinitely completey OTT


	David

-- 
David Laight: david@l8s.co.uk