On Wed, Sep 23, 2015 at 08:26:36AM +1000, Dave Chinner wrote:
> On Tue, Sep 22, 2015 at 10:16:05AM -0500, Bill O'Donnell wrote:
> > > > goto out;
> > > >
> > > > if (!proc_symlink("fs/xfs/stat", NULL,
> > > > - "/sys/fs/xfs/stats/stats"))
> > > > + "/sys/fs/xfs/stats/stats"))
> > >
> > > intentional whitespace changes?
> > >
> > > > goto out_remove_xfs_dir;
> > > >
> > > > #ifdef CONFIG_XFS_QUOTA
> > > > if (!proc_create("fs/xfs/xqmstat", 0, NULL,
> > > > - &xqmstat_proc_fops))
> > > > + &xqmstat_proc_fops))
> > > > goto out_remove_stat_file;
> > > > if (!proc_create("fs/xfs/xqm", 0, NULL,
> > > > - &xqm_proc_fops))
> > > > + &xqm_proc_fops))
> > >
> > > Same question. (did checkpatch complain or something?)
> >
> > Yes. Checkpatch didn't like any spaces, so I turned em into tabs.
>
> Checkpatch should be considered harmful.
>
> It's a good starting point, but it ends up doing more harm than good
> because it doesn't understand the subtle differences in code across
> different subsystems.
>
> In this case, the standard we use for XFS is that multi-line
> parameters should be lined up with the first parameter, unless the
> new parameters don't fit on a single line, and then we back up the
> indent by a tab at a time. i.e. This is correct:
>
> if (!proc_create("fs/xfs/xqm", 0, NULL,
> &xqm_proc_fops))
>
> regardless of what checkpatch says. Indeed, if checkpatch had any
> brains, it would have noticed that a line break is not necessary
> as this:
>
> if (!proc_create("fs/xfs/xqm", 0, NULL, &xqm_proc_fops))
>
> fits in 80 columns just fine.
>
> Remember that Documentation/CodingStyle is a set of guidelines, not
> a strict, enforcable set of rules. The basic guidelines canbe
> summarised as:
>
> 1. Use the same style as the existing code in the file,
> unless reviewers/maintainers ask otherwise.
> 2. new files should follow the format used in other files in
> the subsystem, unless reviewers/maintainer asks otherwise.
> 3. Do not reformat code that you don't need to directly
> modify in the patch.
> 4. modify your editor to highlight common style things that
> you need reminders to get right
> 5. Immolate checkpatch before the plague spreads further
>
> As to #4, there's plenty of simple things you can do. e.g to
> identify stray whitespace in files, add this to your .vimrc file
> (you can do similar with emacs, google it):
>
> " highlight whitespace damage
> highlight RedundantSpaces ctermbg=red guibg=red
> match RedundantSpaces /\s\+$\| \+\ze\t/
>
> This will catch things like "tab-space-tab" and it will also find
> trailing whitespace at the end of lines. They will appear in red.
>
> > > > -#define XFS_STATS_INC(v) (per_cpu(xfsstats, current_cpu()).v++)
> > > > -#define XFS_STATS_DEC(v) (per_cpu(xfsstats, current_cpu()).v--)
> > > > -#define XFS_STATS_ADD(v, inc) (per_cpu(xfsstats, current_cpu()).v +=
> > > > (inc))
> > > > +#define XFS_STATS_INC(v) (per_cpu(*xfsstats.xs_stats,
> > > > current_cpu()).v++)
> > > > +#define XFS_STATS_DEC(v) (per_cpu(*xfsstats.xs_stats,
> > > > current_cpu()).v--)
> > > > +#define XFS_STATS_ADD(v, inc) (per_cpu(*xfsstats.xs_stats,
> > > > current_cpu()).v += (inc))
> > >
> > > Line > 80 chars
> >
> > I'll fix it.
>
> Checkpatch got changed not to warn about that, because too many
> people complained about it when modifying files with >80 column
> formatting.
>
> To that end, I also use custom column width highlights based on file
> names so that line wrapping occurs automatically at the correct
> spot, and when looking at code I can clearly see long lines:
>
> " set the textwidth to 80 characters by default
> set tw=80
>
> " set the textwidth to 68 characters for guilt commit messages
> au BufNewFile,BufRead guilt.msg.*,.gitsendemail*,.git* set tw=68
>
> " set the textwidth to 68 characters for replies (email&usenet)
> au BufNewFile,BufRead .letter,mutt*,nn.*,snd.* set tw=68
>
> " highlight textwidth
> set cc=+1
>
> If you do little things like this, the need for checkpatch goes
> away. With a default tw=80 and a highlight, I only have to glance
> at a patch to know it has lines longer than 80 columns in it.
>
> As such, I haven't used checkpatch for years, and I recommend that
> you develop the right habits and automation such that you don't need
> to use it after a couple of months, either...
All points well taken. I'm fixing it all up in the next patch!
Thanks!
Bill
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
|