lkml.org 
[lkml]   [2004]   [Dec]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 2/3] whitespace cleanups in fs/cifs/file.c
    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


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

    \
     
     \ /
      Last update: 2005-03-22 14:09    [W:0.033 / U:30.572 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site