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

[Jesper Juhl <juhl-lkml@dif.dk>]

On Wed, 29 Dec 2004, Jörn Engel wrote:

> On Wed, 29 December 2004 00:52:32 +0100, Jesper Juhl wrote:
> > -		if(file->private_data != NULL) {
> > +		if (file->private_data != NULL) {
> 
> 		if (file->private_data) {
> 
> It's a question of taste, but in most cases I consider the shorter
> version to be more obvious.
> 
Yeah, matter of personal preference, but since both styles are used in the 
file I had to pick one of them to try and make it consistent - I simply 
picked my personally prefered form.






> > -static int cifs_relock_file(struct cifsFileInfo * cifsFile)
> > +static int 
> > +cifs_relock_file(struct cifsFileInfo *cifsFile)
> 
> Linus viciously prefers to keep return type and function name on a
> single line.  I cannot quite follow his reasoning, but would leave
> that part out, unless explicitly requested by Steve.
> 
Again, this was a matter of trying to achieve internal concistency within 
the file. 


> > -/*	buf = kmalloc(sizeof(FILE_ALL_INFO),GFP_KERNEL);
> > -	if(buf==0) {
> > +/*	buf = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL);
> > +	if (buf==0) {
> 
> Commented out?  Oh well, doesn't hurt either way.
> 
I was going through a few thousand lines I figured I might as well go 
through the commented out bits as well while I was at it ;)


> > @@ -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.




> >  	struct cifsFileInfo *pCFileStruct =
> > -	    (struct cifsFileInfo *) file->private_data;
> > -	char * ptmp;
> > +	    (struct cifsFileInfo *)file->private_data;
> > +	char *ptmp;
> 
> dito
> 
> > @@ -563,7 +565,7 @@ cifs_lock(struct file *file, int cmd, st
> >  
> >  	if (IS_GETLK(cmd)) {
> >  		rc = CIFSSMBLock(xid, pTcon,
> > -				 ((struct cifsFileInfo *) file->
> > +				 ((struct cifsFileInfo *)file->
> >  				  private_data)->netfid,
> >  				 length,
> >  				 pfLock->fl_start, 0, 1, lockType,
> 




> > -	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.


> > -		} else
> > +		} else {
> >  			*poffset += bytes_written;
> > +		}
> 
> Don't.  The extra brackets only cost extra lines on your display.
> Most people are unaware of it, but the square-inch of display area is
> the single most expensive part of most workstations.  Don't waste it.
> 
Yes, they cost screen space, but sometimes they also make things more 
readable even when not strictly needed - when that was (IMO) the case, I 
added the brackets... doesn't matter much - Steve's decition :)


> > -		rc = cifs_write(file, page_data+offset,to-offset,
> > +		rc = cifs_write(file, page_data+offset, to-offset,
> >                                          &position);
> 
> Combining those two lines should still be within the 80 column budget.
> 
Right, I overlooked that (I've probably overlooked a bunch of other stuff 
- there was a lot of lines in there ;)


> > -			memset(target+bytes_read,0,PAGE_CACHE_SIZE-bytes_read);
> > +			memset(target + bytes_read, 0, PAGE_CACHE_SIZE - bytes_read);
> 				     ^ ^			      ^ ^
> I'd remove those four spaces again.  It's very personal, agreed.  Imo,
> the arithmetic operators and the commmata have different precedence on
> the semantical level, so it helps me to have different syntax here as
> well.  But surely many people will disagree and most Just Don't
> Care(tm).
> 
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.


> > -				if(rc != 0)
> > +				if (rc != 0)
> 
> see above
> 
Yup, see above :)


> > -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.


> > -				(FILE_DIRECTORY_INFO *) ((char *) pfindData + 
> > +				(FILE_DIRECTORY_INFO *)((char *) pfindData + 
> >  					le16_to_cpu(findParms.LastNameOffset));
> 
> Nasty casting.  In most cases I fixed similar code by creating a
> helper inline function with a proper name.  Requires some care,
> though.
> 
Sepperate issue, sepperate patch.


> 
> Rest looks fine to me.  Since it's Steve's code, treat this as some
> random opinion only.  Cleanups should make the code more maintainable
> and whoever is the maintainer defines what that means.
> 
Ofcourse. It's all up to Steve.


> Still, a pile of nice work.  Thanks.
> 
Thank you.


-- 
Jesper