Re: [patch 2/3] whitespace cleanups in fs/cifs/file.c

From: Jörn Engel
Date: Wed Dec 29 2004 - 07:31:55 EST


On Wed, 29 December 2004 03:59:55 +0100, Jesper Juhl wrote:
>
> > > @@ -408,7 +410,7 @@ cifs_close(struct inode *inode, struct f
> > > struct cifs_sb_info *cifs_sb;
> > > struct cifsTconInfo *pTcon;
> > > struct cifsFileInfo *pSMBFile =
> > > - (struct cifsFileInfo *) file->private_data;
> > > + (struct cifsFileInfo *)file->private_data;
> >
> > struct cifsFileInfo *pSMBFile = file->private_data;
> >
> > Casting a typeless pointer is pointless.
> >
> This was a 'whitespace fixes only' patch. I have no problem with going
> through the file and looking for pointless casts etc, but that would be a
> sepperate patch.

Sure. I noticed it while going through your patch, that's all. If
you find the time for a second patch, that would be nice. Casts are a
very effective obfuscation method and most are pretty simple to avoid.
Maybe I should check the kernel janitor list and add this point, if
it doesn't exist yet.

> > > - if(file->f_dentry) {
> > > - if(file->f_dentry->d_inode) {
> > > + if (file->f_dentry) {
> > > + if (file->f_dentry->d_inode) {
> >
> > if (file->f_dentry && file->f_dentry->d_inode) {
> >
> > There is too little context to see if this conversion is possible.
> > And I'm too lazy to check myself.
> >
> I didn't check that either since that's not what this patch was about - it
> was strictly formatting/whitespace cleanups and no code changes.

Yup. Same as above, except for the janitor list.

> - there was a lot of lines in there ;)

You can say that again, Mr. Hat!

> I made those changes since (again) both styles are used in the file, so to
> make it consistent I had to choose one of the styles, and picked my
> personal preference - that's the only reason behind that change.

Personal style is hard to argue about. And doesn't make much of a
difference anyway.

> > > -static void reset_resume_key(struct file * dir_file,
> > > - unsigned char * filename,
> > > - unsigned int len,int Unicode,struct nls_table * nls_tab) {
> > > +static void
> > > +reset_resume_key(struct file *dir_file, unsigned char *filename,
> > > + unsigned int len, int Unicode, struct nls_table *nls_tab)
> > > +{
> >
> > Lex Linus? Either way you don't stay within the 80 column.
> >
> Whoops, my bad, I intended to.

Sorry. The whole function declaration is spread over three lines. I
don't mind p***ing Linus off iff putting the return type on a seperate
line is sufficient to fit all the rest into a single line. Doesn't
work here, so you get to argue in favor, not me. ;)

> Sepperate issue, sepperate patch.

Agreed. Google proposes "separate", btw.

Jörn

--
The strong give up and move away, while the weak give up and stay.
-- unknown
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/