The following fails to compile on at least one platform: if (NS_UNLIKELY(frame)); // where 'frame' is a nsIFrame* This is the error I got on the "Linux prometheus" Tinderbox: nsLineBox.cpp:518: error: invalid conversion from `nsIFrame*' to `long int' A bit of info from "prometheus" build log: uname: Linux prometheus.mozilla.org 2.4.21-27.0.4.ELsmp #1 SMP Sat Apr 16 18:43:06 EDT 2005 i686 i686 i386 GNU/Linux Compiler is -- gcc (gcc (GCC) 3.3.2 20031022 (Red Hat Linux 3.3.2-1) My opinion is that if "if(x);" compiles then so should "if(NS_UNLIKELY(x));" for any given x.

Does changing the macro definition to this fix it? #define NS_UNLIKELY(x) (__builtin_expect((x != 0), 0))

I would think so (if you wrap the x in parens), I haven't got access to "prometheus" so I can't test... Here's an alternative that I think would work as well: #define NS_UNLIKELY(x) (__builtin_expect(!!(x), 0))

Tested with gcc 4.1.2 on Linux/x86_64 and gcc 3.3.1 on Linux/i386

I checked this in today but had to back it out because it made SeaMonkey Tinderboxes "Linux lhasa Dep release (gtk2+xft)" and "Linux luna Dep" orange: ======================================================================= ... seamonkey-bin built successfully. seamonkey-bin binary exists, build successful. Running regxpcom test ... Timeout = 120 seconds. Begin: Thu Nov 9 11:04:11 2006 cmd = /builds/tinderbox/SeaMonkey-Release/Linux_2.4.20-28.8_Depend/mozilla//dist/bin/regxpcom End: Thu Nov 9 11:04:17 2006 ----------- Output from regxpcom ------------- ----------- End Output from regxpcom --------- regxpcom: test failed Found profile. Creating clean profile ... Deleting /builds/tinderbox/SeaMonkey-Release/Linux_2.4.20-28.8_Depend/.mozilla/ ... Begin: Thu Nov 9 11:04:17 2006 cmd = /builds/tinderbox/SeaMonkey-Release/Linux_2.4.20-28.8_Depend/mozilla//dist/bin/seamonkey-bin -CreateProfile default End: Thu Nov 9 11:04:21 2006 ----------- Output from Profile Creation ------------- (Gecko:7218): GLib-WARNING **: Priorities can only be increased by root. ----------- End Output from Profile Creation --------- ERROR: couldn't find prefs.js in /builds/tinderbox/SeaMonkey-Release/Linux_2.4.20-28.8_Depend/.mozilla/ no pref file found ======================================================================= After backing it out the Tinderboxes went green again so it was this patch that caused the problem although I can't understand why right now... (there was no problem on Firefox or Camino Tinderboxes that runs the same test.)

__builtin_expect returns its first arg (type long): http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-g_t_005f_005fbuiltin_005fexpect-2455 NS_(UN)LIKELY returns what __builtin_expect returns. NS_FAILED returns what NS_UNLIKELY returns: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/base/nsError.h&rev=1.49&root=/cvsroot&mark=113#112 The patch gives a different result when assigning the NS_FAILED(rv) result to a PRPackedBool (= PRUint8), the Tinderbox orange was caused by: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/modules/libpref/src/nsPrefService.cpp&rev=1.91&root=/cvsroot&mark=375#358 (The error probably occurs for all assignments to a variable of integral type narrower than 'long'.) This test: PRBool old1 = __builtin_expect((0x80000000), 0); PRPackedBool old2 = __builtin_expect((0x80000000), 0); PRBool new1 = __builtin_expect(!!(0x80000000), 0); PRPackedBool new2 = __builtin_expect(!!(0x80000000), 0); printf("old(%d %d) new(%d %d)\n", old1, old2, new1, new2); prints: old(-2147483648 0) new(1 1) I think the current definition of NS_FAILED is to blame. Shouldn't it return 0 or 1, like NS_SUCCEEDED? The nsPrefService code seems to depend on this bug though...

> Shouldn't it return 0 or 1, like NS_SUCCEEDED? In my opinion, yes! > The nsPrefService code seems to depend on this bug though... The mErrorOpeningUserPrefs thing? You mean the fact that it's always 0 currently? Seems to me like that's just a bug we should fix, no? Or just nix that member, if it's not useful anyway.

Comment on attachment 245049 [details] [diff] [review] Patch rev. 1 You should really fix the patch to put the !! in the #else-half as well, so bugs like this show up in all builds rather than just some.

1. add !! to the #else part 2. remove the always-false flags in nsPrefService - I will file a follow-up bug to restore them because I think not overwriting an existing prefs.js when there are read errors seems like a good idea (assuming this was the intention). 3. fixed some dubious tests in some DEBUG-only editor code that assumes the value of NS_FAILED(rv) == NS_OK when 'rv' is the result of a successful operation 4. I added comments to NS_FAILED/SUCCEEDED and NS_(UN)LIKELY making it clear that they are guaranteed to return 0 or 1 now I tested this on several platforms. I also compiled a version where I made these macros return a class object with a private 'operator char' to catch all assignments to PR(Packed)Bool (for the configuration I built at least) and nsPrefService was the only error. (I found a few 'PRPackedBool initialized = NS_SUCCEEDED(rv)' but those should be ok.) BTW, I think the recommended usage example in the comment encourages code that doesn't follow the coding rules: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/base/nscore.h&rev=1.98&root=/cvsroot&mark=451-457#447 Wouldn't the following be better? * if (NS_LIKELY(v)) { * ... expected code path ... * } * * if (NS_UNLIKELY(!v)) { * ... non-expected code path ... * } which covers the following common cases: if (NS_LIKELY(pointer)) { ... if (NS_UNLIKELY(!pointer)) { ... which is preferred over if (NS_LIKELY(pointer != nsnull)) { ... if (NS_UNLIKELY(pointer == nsnull)) { ...

Checked in to trunk at 2006-11-17 16:48 PST Filed bug 361102 on fixing point 2. above -> FIXED