Re: [PATCH] fix threaded grep for machines with only one cpu

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 16, 2010, 19:20 UTC
Message-ID: <7vocjpng1w.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20100216180209.GA1532@book.hvoigt.net>

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 5 quoted lines

>> IOW, please try this patch.  I am planning to queue it to 'maint' as part
>> of 1.7.0.1 if this is the right solution (which I obviously think it is).
>
> Yes your patch does it correctly I just verified that the segfaults are
> gone as well. I think your solution is even nicer than mine. Thanks.

Show 8 quoted lines

>> +#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
>> +#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)
>
> One minor thing: Would it not be even nicer having the while loop inside
> the if clause? E.g like this
>
> #define read_sha1_lock() if (use_threads) do { pthread_mutex_lock(&read_sha1_mutex); } while (0)
> #define read_sha1_unlock() if (use_threads) do { pthread_mutex_unlock(&read_sha1_mutex); } while (0)

No.

	#define frotz() do { this compound stmt; } while (0)

is a common idiom to make a macro that expands to a compound stmt behave as if it is a simple function call to avoid bugs when it is expanded, regardless of in which context it is used by an unsuspecting caller.

Your rewrite is pointless because it is the same as saying

	#define read_sha1_lock() if (use_threads) p_m_l(&r_s_m)

and that is exactly what the idiom's use of "do { } while (0)" is all about.

Try this simple program.

-- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 -- #include <stdio.h>

#define frotz() do { if (flag) printf("frotz"); } while (0) #define xyzzy() if (flag) do { printf("xyzzy"); } while (0) #define yomin() if (flag) printf("yomin")

void test(int foo, int bar, int baz, int flag)
{
        if (foo)
                frotz();
        else if (bar)
                xyzzy();
        else if (baz)
                yomin();
        else
                printf("huh?");

        printf("\n");
}

int main(int ac, char **av)
{
        int foo, bar, baz;
        for (foo = 0; foo < 2; foo++)
                for (bar = 0; bar < 2; bar++)
                        for (baz = 0; baz < 2; baz++) {
                                printf("%d %d %d ", foo, bar, baz);
                                test(foo, bar, baz, 1);
                        }
        return 0;
}
-- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 --

Textually the "test" function expands to this:

        void test(int foo, int bar, int baz, int flag)
        {
         if (foo)
          do { if (flag) printf("frotz"); } while (0);
         else if (bar)
          if (flag) do { printf("xyzzy"); } while (0);
         else if (baz)
          if (flag) printf("yomin");
         else
          printf("huh?");

         printf("\n");
        }

but if you properly indent it, it looks like this:

        void test(int foo, int bar, int baz, int flag)
        {
                if (foo)
                        do {
                                if (flag)
                                        printf("frotz");
                        } while (0);
                else if (bar)
                        if (flag)
                                do {
                                        printf("xyzzy");
                                } while (0);
                        else if (baz)
                                if (flag)
                                        printf("yomin");
                                else
                                        printf("huh?");
                printf("\n");
        }

Notice how your version (xyzzy) broke the cascade of if..elseif..else.

Don't they teach this in schools anymore?