Hi Dave, On 5/19/23 16:25, Dave Chinner wrote: > On Fri, May 19, 2023 at 08:13:50AM -0700, Randy Dunlap wrote: >> >> >> On 5/19/23 02:28, Bagas Sanjaya wrote: >>>> +/** >>>> + * DOC: Flags reported by the file system from iomap_begin >>>> * >>>> - * IOMAP_F_NEW indicates that the blocks have been newly allocated and need >>>> - * zeroing for areas that no data is copied to. >>>> + * * IOMAP_F_NEW: indicates that the blocks have been newly allocated and need >>>> + * zeroing for areas that no data is copied to. >>>> * >>>> - * IOMAP_F_DIRTY indicates the inode has uncommitted metadata needed to access >>>> - * written data and requires fdatasync to commit them to persistent storage. >>>> - * This needs to take into account metadata changes that *may* be made at IO >>>> - * completion, such as file size updates from direct IO. >>>> + * * IOMAP_F_DIRTY: indicates the inode has uncommitted metadata needed to access >>>> + * written data and requires fdatasync to commit them to persistent storage. >>>> + * This needs to take into account metadata changes that *may* be made at IO >>>> + * completion, such as file size updates from direct IO. >>>> * >>>> - * IOMAP_F_SHARED indicates that the blocks are shared, and will need to be >>>> - * unshared as part a write. >>>> + * * IOMAP_F_SHARED: indicates that the blocks are shared, and will need to be >>>> + * unshared as part a write. >>>> * >>>> - * IOMAP_F_MERGED indicates that the iomap contains the merge of multiple block >>>> - * mappings. >>>> + * * IOMAP_F_MERGED: indicates that the iomap contains the merge of multiple block >>>> + * mappings. >>>> * >>>> - * IOMAP_F_BUFFER_HEAD indicates that the file system requires the use of >>>> - * buffer heads for this mapping. >>>> + * * IOMAP_F_BUFFER_HEAD: indicates that the file system requires the use of >>>> + * buffer heads for this mapping. >>>> * >>>> - * IOMAP_F_XATTR indicates that the iomap is for an extended attribute extent >>>> - * rather than a file data extent. >>>> + * * IOMAP_F_XATTR: indicates that the iomap is for an extended attribute extent >>>> + * rather than a file data extent. >>>> */ >>> Why don't use kernel-doc comments to describe flags? >>> >> >> Because kernel-doc handles functions, structs, unions, and enums. >> Not defines. > > So perhaps that should be fixed first? > > I seriously dislike the implication here that we should accept > poorly/inconsistently written comments and code just to work around > deficiencies in documentation tooling. > > Either modify the code to work cleanly and consistently with the > tooling (e.g. change the code to use enums rather than #defines), or > fix the tools that don't work with macro definitions in a way that > matches the existing code documentation standards. > > Forcing developers, reviewers and maintainers to understand, accept > and then maintain inconsistent crap in the code just because some > tool they never use is deficient is pretty much my definition of an > unacceptible engineering process. I started looking into this. It looks like Mauro added more support to scripts/kernel-doc for typedefs and macros while I wasn't looking. I don't know how well it works, but it's not clear to me how we would want this to look. For example, take the first set of defines from <linux/iomap.h> that Luis modified: /* * Types of block ranges for iomap mappings: */ #define IOMAP_HOLE 0 /* no blocks allocated, need allocation */ #define IOMAP_DELALLOC 1 /* delayed allocation blocks */ #define IOMAP_MAPPED 2 /* blocks allocated at @addr */ #define IOMAP_UNWRITTEN 3 /* blocks allocated at @addr in unwritten state */ #define IOMAP_INLINE 4 /* data inline in the inode */ and Luis changed that to: /** * DOC: iomap block ranges types * * * IOMAP_HOLE - no blocks allocated, need allocation * * IOMAP_DELALLOC - delayed allocation blocks * * IOMAP_MAPPED - blocks allocated at @addr * * IOMAP_UNWRITTEN - blocks allocated at @addr in unwritten state * * IOMAP_INLINE - data inline in the inode */ How would we want that to look? How would we express it in kernel-doc format? Currently it would have to be one macro at a time I think. /* * Types of block ranges for iomap mappings: */ /** * IOMAP_HOLE - no blocks allocated, need allocation */ #define IOMAP_HOLE 0 /** * IOMAP_DELALLOC - delayed allocation blocks */ #define IOMAP_DELALLOC 1 /** * IOMAP_MAPPED - blocks allocated at @addr */ #define IOMAP_MAPPED 2 /** * IOMAP_UNWRITTEN - blocks allocated at @addr in unwritten state */ #define IOMAP_UNWRITTEN 3 /** * IOMAP_INLINE - data inline in the inode */ #define IOMAP_INLINE 4 That's ugly to my eyes. And it doesn't collect the set of macros together in any manner. The modification that Luis made looks pretty good to me ATM. -- ~Randy
