lkml.org 
[lkml]   [2011]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] vmalloc: remove #ifdef in function body
    On Tue, Dec 20, 2011 at 09:31:21PM -0800, Joe Perches wrote:
    > On Wed, 2011-12-21 at 14:17 +0900, Minchan Kim wrote:
    > > We don't like function body which include #ifdef.
    > []
    > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
    > []
    > > @@ -505,6 +505,7 @@ static void unmap_vmap_area(struct vmap_area *va)
    > > vunmap_page_range(va->va_start, va->va_end);
    > > }
    > >
    > > +#ifdef CONFIG_DEBUG_PAGEALLOC
    > > static void vmap_debug_free_range(unsigned long start, unsigned long end)
    > > {
    > > /*
    > > @@ -520,11 +521,15 @@ static void vmap_debug_free_range(unsigned long start, unsigned long end)
    > > * debugging doesn't do a broadcast TLB flush so it is a lot
    > > * faster).
    > > */
    > > -#ifdef CONFIG_DEBUG_PAGEALLOC
    > > vunmap_page_range(start, end);
    > > flush_tlb_kernel_range(start, end);
    > > -#endif
    > > }
    > > +#else
    > > +static inline void vmap_debug_free_range(unsigned long start,
    > > + unsigned long end)
    > > +{
    > > +}
    > > +#endif
    >
    > I don't like this change.
    > I think it's perfectly good style to use:

    I feel it's no problem as it is because it's very short function now
    but it's not style we prefer.

    >
    > 1 void foo(args...)
    > 2 {
    > 3 #ifdef CONFIG_FOO
    > 4 ...
    > 5 #endif
    > 6 }
    >
    > instead of
    >
    > 1 #ifdef CONFIG_FOO
    > 2 void foo(args...)
    > 3 {
    > 4 ...
    > 5 }
    > 6 #else
    > 7 void foo(args...)
    > 8 {
    > 9 }
    > 10 #endif
    >
    > The first version is shorter and gcc optimizes
    > away the void func just fine. It also means

    Agree but if function would be long(but I convice
    it's not long in future :)), it would be messy.

    > that 2 function prototypes don't need to be
    > kept in agreement when someone changes one
    > without testing CONFIG_FOO=y and CONFIG_FOO=n.

    The goal is not for making test easily.
    Patch author should keep it consistent.

    This patch is just trivial so I don't mind if who have
    against this patch strongly. What I want to say is
    it's not style we prefer.

    >
    >

    --
    Kind regards,
    Minchan Kim


    \
     
     \ /
      Last update: 2011-12-21 06:47    [W:0.022 / U:60.148 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site