linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Randy Dunlap <randy.dunlap@oracle.com>
To: David Howells <dhowells@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	trond.myklebust@fys.uio.no, viro@zeniv.linux.org.uk,
	nfsv4@linux-nfs.org, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 24/45] CacheFiles: Add a hook to write a single page of data to an inode [ver #41]
Date: Fri, 21 Nov 2008 09:36:44 -0800	[thread overview]
Message-ID: <4926F1AC.7090301@oracle.com> (raw)
In-Reply-To: <3843.1227271421@redhat.com>

David Howells wrote:
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>> Sigh.  I don't normally comment on busted comment layout, but Ingo does
>> so I'm allowed to ;) See recent discussion around the kmemleak patches.
> 
> Sigh.  Three points for you to consider:
> 
>  (1) The prevailing comment style in struct address_space_operations does not
>      have comment delimiters on their own lines.  This has the advantage that
>      it doesn't unnecessarily waste two lines per comment.  I will advocate
>      that style for banner comments for top-level constructs, but for internal
>      comments it's generally a poor compromise.

I would have said that the prevailing comment style in struct
address_space_operations is sketchy or limited or lacking.

I want useful comments.  I don't give a hoot whether they are in kernel-doc
format or /* or //.  I'll even take them in a separate file if they are
more about theory of operation (overview) instead of directly related
to a struct or a function's operations, although some people want comments
only in source files because that is more likely to be updated when
needed.  I agree with that sentiment, but I wouldn't limit people to
having comments only in source files.  I just want comments or doc files
(which you usually provide plenty of).


>      The following comment on braces applies also to comments:
> 
> 	Also, note that this brace-placement also minimizes the number of empty
> 	(or almost empty) lines, without any loss of readability.  Thus, as the
> 	supply of new-lines on your screen is not a renewable resource (think
> 	25-line terminal screens here), you have more empty lines to put
> 	comments on.
> 
>  (2) Randy Dunlap should be beaten for his contribution to CodingStyle.  Next
>      time I have the opportunity, I'll do just that, and this time I'll see
>      about using one of my own racquets rather than one of his:-P

Bring it on.  and -ENOPATCH.

I don't own Documentation/CodingStyle. I'm just willing to send patches for it.
You (or anyone) can do the same.


>  (3) It's so much easier to write style guides than to properly document one's
>      code.

Easy to write a style guide, but difficult to get consensus on it.

If you don't document your own code, who will do it?  Probably nobody.


> </rant>
> 
>>> +/**
>>> + * generic_file_buffered_write_one_page - Write a single page of data to an
>>> + *	inode
>> kerneldoc doesn't permit line breaks in this context (unless it got
>> fixed recently)

No, it's not fixed.

> So you advocate a line exceeding 80 chars?

Either that or break the first line (function name & short description) up like:

/**
 * fubar - generally scrog and muck the system [based on an armed forces acronym for guess what]
 */

Change that to:

/**
 * fubar - scrog and muck up the system
 *
 * fubar (the name) is believed to come from an armed forces acronym
 * for you_know_what.
 */


> I contend that kerneldoc is a bad idea in many ways: it encourages people to
> be lazy and to not write proper interface documentation.

I can't/don't force you or anyone to write proper documentation, and the
maintainers who could do that don't do it.

But basically (see above), I just want documentation.  It's form is
not a big deal to me.  The good thing about kernel-doc is that it
provides some consistency.  Have you ever heard of look and feel?
Well, documentation can have consistent look and feel too, and it should.

Do we make exceptions?  Sure, we do.  Just look at Documentation/lguest/.


-- 
~Randy

  parent reply	other threads:[~2008-11-21 17:37 UTC|newest]

Thread overview: 83+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-11-20 14:41 [PATCH 00/45] Permit filesystem local caching [ver #41] David Howells
2008-11-20 14:41 ` [PATCH 01/45] Create a dynamically sized pool of threads for doing very slow work items " David Howells
2008-11-21  8:09   ` Andrew Morton
2008-11-21 10:24   ` David Howells
2008-11-21 18:17     ` Andrew Morton
2008-11-22  0:38     ` David Howells
2008-12-19  4:14   ` Serge E. Hallyn
2008-12-19  4:19   ` Serge E. Hallyn
2008-12-19  7:15   ` Andrew Morton
2008-12-19 11:42   ` David Howells
2008-12-19 11:44   ` David Howells
2008-12-19 12:12   ` David Howells
2008-12-19 16:52     ` Serge E. Hallyn
2008-12-19 12:54   ` David Howells
2008-11-20 14:41 ` [PATCH 02/45] Make slow-work thread pool actually dynamic " David Howells
2008-12-19 17:58   ` Serge E. Hallyn
2008-11-20 14:41 ` [PATCH 03/45] Make the slow work pool configurable " David Howells
2008-12-19 18:33   ` Serge E. Hallyn
2008-11-20 14:42 ` [PATCH 04/45] Document the slow work thread pool " David Howells
2008-11-20 14:42 ` [PATCH 05/45] FS-Cache: Release page->private after failed readahead " David Howells
2008-11-21  8:12   ` Andrew Morton
2008-11-21 10:27   ` David Howells
2008-11-20 14:42 ` [PATCH 06/45] FS-Cache: Recruit a couple of page flags for cache management " David Howells
2008-11-21  8:17   ` Andrew Morton
2008-11-21 10:31   ` David Howells
2008-11-20 14:42 ` [PATCH 07/45] FS-Cache: Provide an add_wait_queue_tail() function " David Howells
2008-11-21  8:17   ` Andrew Morton
2008-11-21 13:32   ` David Howells
2008-11-20 14:42 ` [PATCH 08/45] FS-Cache: Add the FS-Cache netfs API and documentation " David Howells
2008-11-20 14:42 ` [PATCH 09/45] FS-Cache: Add the FS-Cache cache backend " David Howells
2008-11-20 14:42 ` [PATCH 10/45] FS-Cache: Add main configuration option, module entry points and debugging " David Howells
2008-11-20 14:42 ` [PATCH 11/45] FS-Cache: Add use of /proc and presentation of statistics " David Howells
2008-11-21  0:15   ` Alexey Dobriyan
2008-11-21  2:17   ` David Howells
2008-11-21  2:34     ` Alexey Dobriyan
2008-11-21 15:32   ` David Howells
2008-11-20 14:42 ` [PATCH 12/45] FS-Cache: Root index definition " David Howells
2008-11-20 14:42 ` [PATCH 13/45] FS-Cache: Add cache tag handling " David Howells
2008-11-20 14:42 ` [PATCH 14/45] FS-Cache: Add cache management " David Howells
2008-11-20 14:42 ` [PATCH 15/45] FS-Cache: Provide a slab for cookie allocation " David Howells
2008-11-20 14:43 ` [PATCH 16/45] FS-Cache: Add netfs registration " David Howells
2008-11-20 14:43 ` [PATCH 17/45] FS-Cache: Bit waiting helpers " David Howells
2008-11-20 14:43 ` [PATCH 18/45] FS-Cache: Object management state machine " David Howells
2008-11-20 14:43 ` [PATCH 19/45] FS-Cache: Implement the cookie management part of the netfs API " David Howells
2008-11-20 14:43 ` [PATCH 20/45] FS-Cache: Add and document asynchronous operation handling " David Howells
2008-11-20 14:43 ` [PATCH 21/45] FS-Cache: Implement data I/O part of netfs API " David Howells
2008-11-20 14:43 ` [PATCH 22/45] CacheFiles: Add missing copy_page export for ia64 " David Howells
2008-11-20 14:43 ` [PATCH 23/45] CacheFiles: Be consistent about the use of mapping vs file->f_mapping in Ext3 " David Howells
2008-11-22 17:38   ` Andreas Dilger
2008-11-26 14:40   ` David Howells
2008-11-20 14:43 ` [PATCH 24/45] CacheFiles: Add a hook to write a single page of data to an inode " David Howells
2008-11-21  8:23   ` Andrew Morton
2008-11-21 12:43   ` David Howells
2008-11-21 13:00     ` Jamie Lokier
2008-11-21 17:15       ` Valdis.Kletnieks
2008-11-21 17:36     ` Randy Dunlap [this message]
2008-11-21 18:31     ` Andrew Morton
2008-11-22  0:48     ` David Howells
2008-11-20 14:43 ` [PATCH 25/45] CacheFiles: Permit the page lock state to be monitored " David Howells
2008-11-20 14:43 ` [PATCH 26/45] CacheFiles: Export things for CacheFiles " David Howells
2008-11-20 14:43 ` [PATCH 27/45] CacheFiles: A cache that backs onto a mounted filesystem " David Howells
2008-11-20 14:44 ` [PATCH 28/45] FS-Cache: Make kAFS use FS-Cache " David Howells
2008-11-20 14:44 ` [PATCH 29/45] NFS: Add comment banners to some NFS functions " David Howells
2008-11-20 14:44 ` [PATCH 30/45] NFS: Add FS-Cache option bit and debug bit " David Howells
2008-11-20 14:44 ` [PATCH 31/45] NFS: Permit local filesystem caching to be enabled for NFS " David Howells
2008-11-20 14:44 ` [PATCH 32/45] NFS: Register NFS for caching and retrieve the top-level index " David Howells
2008-11-20 14:44 ` [PATCH 33/45] NFS: Define and create server-level objects " David Howells
2008-11-20 14:44 ` [PATCH 34/45] NFS: Define and create superblock-level " David Howells
2008-11-20 14:44 ` [PATCH 35/45] NFS: Define and create inode-level cache " David Howells
2008-11-20 14:44 ` [PATCH 36/45] NFS: Use local disk inode cache " David Howells
2008-11-20 14:44 ` [PATCH 37/45] NFS: Invalidate FsCache page flags when cache removed " David Howells
2008-11-20 14:44 ` [PATCH 38/45] NFS: Add some new I/O counters for FS-Cache doing things for NFS " David Howells
2008-11-20 14:45 ` [PATCH 39/45] NFS: FS-Cache page management " David Howells
2008-11-20 14:45 ` [PATCH 40/45] NFS: Add read context retention for FS-Cache to call back with " David Howells
2008-11-20 14:45 ` [PATCH 41/45] NFS: nfs_readpage_async() needs to be accessible as a fallback for local caching " David Howells
2008-11-20 14:45 ` [PATCH 42/45] NFS: Read pages from FS-Cache into an NFS inode " David Howells
2008-11-20 14:45 ` [PATCH 43/45] NFS: Store pages from an NFS inode into a local cache " David Howells
2008-11-20 14:45 ` [PATCH 44/45] NFS: Display local caching state " David Howells
2008-11-20 14:45 ` [PATCH 45/45] NFS: Add mount options to enable local caching on NFS " David Howells
2008-11-21  8:28 ` [PATCH 00/45] Permit filesystem local caching " Andrew Morton
2008-11-22  1:11 ` David Howells
2008-11-25  0:09 ` David Howells
2008-11-25 13:39 ` FS-Cache Benchmarks David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4926F1AC.7090301@oracle.com \
    --to=randy.dunlap@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=dhowells@redhat.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nfsv4@linux-nfs.org \
    --cc=trond.myklebust@fys.uio.no \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).