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

[Jörn Engel]

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/