Re: [Pull] Some documentation patches

[Dmitri Vorobiev <dmitri.vorobiev@gmail.com>]

Jan Engelhardt пишет:
> 
> On Friday 2008-03-28 19:20, Jonathan Corbet wrote:
>> commit 9756ccfda31b4c4544aa010aacf71b6672d668e8
>> Date:   Fri Mar 28 11:19:56 2008 -0600
>>
>>    Add the seq_file documentation
> 
> patch on top:
> 
>  - add const qualifiers
>  - remove void* casts
>  - use proper specifier (%Ld is not valid)
> 
> Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>
> 
> diff --git a/Documentation/filesystems/seq_file.txt
> b/Documentation/filesystems/seq_file.txt
> index 92975ee..cc6cdb9 100644
> --- a/Documentation/filesystems/seq_file.txt
> +++ b/Documentation/filesystems/seq_file.txt
> @@ -107,8 +107,8 @@ complete. Here's the example version:
> 
>      static void *ct_seq_next(struct seq_file *s, void *v, loff_t *pos)
>      {
> -            loff_t *spos = (loff_t *) v;
> -            *pos = ++(*spos);
> +            loff_t *spos = v;
> +            *pos = ++*spos;

Excuse me, what's the point in this change and the next one? IMO, removing
the explicit type cast makes the code less obvious (AFAICT, this is a trendy
word in LKML these days). Relying upon operator priorities instead of explicit
operator grouping using parentheses can confuse people, too. Imagine a
person looking at these lines: after the change, he or she will need to check
the variable v type in the argument list, and consult the table of operator
priorities in C if the person is in doubt about what the code does.

Just my two cents...

Dmitri

>              return spos;
>      }
> 
> @@ -127,8 +127,8 @@ something goes wrong. The example module's show()
> function is:
> 
>      static int ct_seq_show(struct seq_file *s, void *v)
>      {
> -            loff_t *spos = (loff_t *) v;
> -            seq_printf(s, "%Ld\n", *spos);
> +            loff_t *spos = v;
> +            seq_printf(s, "%lld\n", (long long)*spos);
>              return 0;
>      }
> 
> @@ -136,7 +136,7 @@ We will look at seq_printf() in a moment. But first,
> the definition of the
>  seq_file iterator is finished by creating a seq_operations structure with
>  the four functions we have just defined:
> 
> -    static struct seq_operations ct_seq_ops = {
> +    static const struct seq_operations ct_seq_ops = {
>              .start = ct_seq_start,
>              .next  = ct_seq_next,
>              .stop  = ct_seq_stop,
> @@ -204,7 +204,7 @@ line, as in the example module:
>      static int ct_open(struct inode *inode, struct file *file)
>      {
>          return seq_open(file, &ct_seq_ops);
> -    };
> +    }
> 
>  Here, the call to seq_open() takes the seq_operations structure we created
>  before, and gets set up to iterate through the virtual file.
> @@ -219,7 +219,7 @@ The other operations of interest - read(), llseek(),
> and release() - are
>  all implemented by the seq_file code itself. So a virtual file's
>  file_operations structure will look like:
> 
> -    static struct file_operations ct_file_ops = {
> +    static const struct file_operations ct_file_ops = {
>              .owner   = THIS_MODULE,
>              .open    = ct_open,
>              .read    = seq_read,
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>