lkml.org 
[lkml]   [2015]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 26/40] arch/sparc: uaccess_64 macro whitespace fixes
    On Tue, Jan 06, 2015 at 05:53:39PM +0100, Sam Ravnborg wrote:
    > On Tue, Jan 06, 2015 at 05:44:56PM +0200, Michael S. Tsirkin wrote:
    > > Macros within arch/sparc/include/asm/uaccess_64.h are made harder to
    > > read because they violate a bunch of coding style rules.
    > >
    > > Fix it up.
    > As per Davem's earlier mail please prefix using sparc32/sparc64.

    I did put in uaccess_64 - insufficient?

    > > -#define __put_user_nocheck(data,addr,size) ({ \
    > > -register int __pu_ret; \
    > > -switch (size) { \
    > > -case 1: __put_user_asm(data,b,addr,__pu_ret); break; \
    > > -case 2: __put_user_asm(data,h,addr,__pu_ret); break; \
    > > -case 4: __put_user_asm(data,w,addr,__pu_ret); break; \
    > > -case 8: __put_user_asm(data,x,addr,__pu_ret); break; \
    > > -default: __pu_ret = __put_user_bad(); break; \
    > > -} __pu_ret; })
    > > -
    > > -#define __put_user_asm(x,size,addr,ret) \
    > > +#define __put_user_nocheck(data, addr, size) ({ \
    > > + register int __pu_ret; \
    > > + switch (size) { \
    > > + case 1: \
    > > + __put_user_asm(data, b, addr, __pu_ret); \
    > > + break; \
    > > + case 2: \
    > > + __put_user_asm(data, h, addr, __pu_ret); \
    > > + break; \
    > > + case 4: \
    > > + __put_user_asm(data, w, addr, __pu_ret); \
    > > + break; \
    > > + case 8: \
    > > + __put_user_asm(data, x, addr, __pu_ret); \
    > > + break; \
    > > + default: \
    > > + __pu_ret = __put_user_bad(); \
    > > + break; \
    > > + } \
    > > + __pu_ret; \
    > > +})
    >
    > No matter what coding style says - the above is much less readable than the
    > original version.
    >
    >
    I guess you approve the rest of the changes then?


    I get it you like it that
    case 1: __get_user_asm(__gu_val,ub,addr,__gu_ret); break;
    has the whole case on the same line?
    Is that the issue?


    >
    > > -#define __get_user_nocheck(data,addr,size,type) ({ \
    > > -register int __gu_ret; \
    > > -register unsigned long __gu_val; \
    > > -switch (size) { \
    > > -case 1: __get_user_asm(__gu_val,ub,addr,__gu_ret); break; \
    > > -case 2: __get_user_asm(__gu_val,uh,addr,__gu_ret); break; \
    > > -case 4: __get_user_asm(__gu_val,uw,addr,__gu_ret); break; \
    > > -case 8: __get_user_asm(__gu_val,x,addr,__gu_ret); break; \
    > > -default: __gu_val = 0; __gu_ret = __get_user_bad(); break; \
    > > -} data = (__force type) __gu_val; __gu_ret; })
    > > -
    > > -#define __get_user_nocheck_ret(data,addr,size,type,retval) ({ \
    > > -register unsigned long __gu_val __asm__ ("l1"); \
    > > -switch (size) { \
    > > -case 1: __get_user_asm_ret(__gu_val,ub,addr,retval); break; \
    > > -case 2: __get_user_asm_ret(__gu_val,uh,addr,retval); break; \
    > > -case 4: __get_user_asm_ret(__gu_val,uw,addr,retval); break; \
    > > -case 8: __get_user_asm_ret(__gu_val,x,addr,retval); break; \
    > > -default: if (__get_user_bad()) return retval; \
    > > -} data = (__force type) __gu_val; })
    > > -
    > > -#define __get_user_asm(x,size,addr,ret) \
    > > +#define __get_user_nocheck(data, addr, size, type) ({ \
    > > + register int __gu_ret; \
    > > + register unsigned long __gu_val; \
    > > + switch (size) { \
    > > + case 1: \
    > > + __get_user_asm(__gu_val, ub, addr, __gu_ret); \
    > > + break; \
    > > + case 2: \
    > > + __get_user_asm(__gu_val, uh, addr, __gu_ret); \
    > > + break; \
    > > + case 4: \
    > > + __get_user_asm(__gu_val, uw, addr, __gu_ret); \
    > > + break; \
    > > + case 8: \
    > > + __get_user_asm(__gu_val, x, addr, __gu_ret); \
    > > + break; \
    > > + default: \
    > > + __gu_val = 0; \
    > > + __gu_ret = __get_user_bad(); \
    > > + break; \
    > > + } data = (__force type) __gu_val; __gu_ret; \
    > > +})
    > > +
    > > +#define __get_user_nocheck_ret(data, addr, size, type, retval) ({ \
    > > + register unsigned long __gu_val __asm__ ("l1"); \
    > > + switch (size) { \
    > > + case 1: \
    > > + __get_user_asm_ret(__gu_val, ub, addr, retval); \
    > > + break; \
    > > + case 2: \
    > > + __get_user_asm_ret(__gu_val, uh, addr, retval); \
    > > + break; \
    > > + case 4: \
    > > + __get_user_asm_ret(__gu_val, uw, addr, retval); \
    > > + break; \
    > > + case 8: \
    > > + __get_user_asm_ret(__gu_val, x, addr, retval); \
    > > + break; \
    > > + default: \
    > > + if (__get_user_bad()) \
    > > + return retval; \
    > > + } \
    > > + data = (__force type) __gu_val; \
    > > +})
    > > +
    >
    > Same comment for this code chunk.
    >
    > Sam

    Well I donnu. When I had to fix bugs there, it was pretty confusing to
    me, conding style is no a holy book but it's there for a reason.

    Lack of spaces after comma makes it so much harder
    to count parameters.

    Also:

    > > -default: if (__get_user_bad()) return retval; \
    > > -} data = (__force type) __gu_val; })

    return on same line with if and code after the closing {}
    makes it look confusingly like the more conventional:

    if (__get_user_bad())
    data = (__force type) __gu_val;

    --
    MST


    \
     
     \ /
      Last update: 2015-01-06 18:41    [W:4.289 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site