there are a number of warnings which we can quiet by annotating that an assignment is limited to debug statements. In order to do this, I need a macro which supports: NS_DEBUG_ASSIGN(lhs) method();

Comment on attachment 456726 [details] [diff] [review] declaration Excuse the drive-by comment... concerning this: >+#define NS_DEBUG_ASSIGN(x) PR_BEGIN_MACRO /* nothing */ PR_END_MACRO; it seems wrong to let the macro introduce a semicolon here. Not sure how I feel about the higher-level issue of whether this construct is desirable in the first place. Actually, the more I think about it, the less I like it. It seems to introduce a new opportunity to unintentionally create subtle debug-vs-release differences in our code.

the semicolon is necessary because of the way PR_BEGIN_MACRO/PR_END_MACRO work. they result in: 192 #define PR_BEGIN_MACRO do { 193 #define PR_END_MACRO } while (0) so a ; is needed. you either get: do {} while (0); bar->Foo(); or: nsresult rv = bar->Foo();

I suppose using just "/* nothing */" would be better, as it'd enable: if (baz) NS_DEBUG_ASSIGN(rv) bar->Foo();

IMHO this should be 1 bug not ~20

This is ugly as sin. NS_DEBUG_ASSIGN(rv, bar->Foo()) looks much nicer to me.

(In reply to comment #6) > This is ugly as sin. NS_DEBUG_ASSIGN(rv, bar->Foo()) looks much nicer to me. The point is that bar->Foo() needs to be evaluated in *both* debug & opt builds, and your suggested syntax in comment 6 would make that less clear, IMHO. If it's outside of the macro (as it is in timeless's suggested syntax here), it's clear that it always will be evaluated.

(In reply to comment #7) > (In reply to comment #6) > > This is ugly as sin. NS_DEBUG_ASSIGN(rv, bar->Foo()) looks much nicer to me. Agreed, but.... > The point is that bar->Foo() needs to be evaluated in *both* debug & opt > builds, and your suggested syntax in comment 6 would make that less clear, > IMHO. ....also agreed. :( In either case, I'm still uncomfortable with the way this macro obscures a debug-vs-opt behavioral change that affects following code. Imagine a long function including T v = 0; .... NS_DEBUG_ASSIGN(v) foo->bar(); NS_WARN_IF_FALSE(v > 0, "expected v to be greater than 0"); .... So far so good. But later someone adds a call such as foo->baz(v); further down the function, without noticing that the contents of v at this point is dependent on the type of build. Of course it's possible to create such bugs already with conditionally-compiled fragments, but ISTM this macro would make it easier to inadvertently make such errors. Would it be preferable to provide a macro such as #define NS_UNUSED_IF_RELEASE __attribute__((unused)) and then append this to the variable declaration concerned?

so, I think that it would be relatively hard to make that mistake based on the way the macro is written - it's *LOUD*. Anyone who wants to use a variable needs to decide why they want to use it, which means looking to see who assigned to it which value. Reviewers could also force people to make their variable declarations NS_DEBUG_DECL (this doesn't exist yet, but we could create it) or #ifdef DEBUG which should make it even harder. So far all of the changes I've made were of the form where the declaration was guarded by the macro - which means anyone trying to use it in a release build will immediately fall over because the variable isn't defined. I've been working on mozilla for over a decade mostly paying attention to quirks and strange logic/programming errors, and conditional compilation bugs are incredibly rare - I can't think of the last such instance. And it isn't because we have very little macro magic or because our code is absolutely straightforward. The primary macro class I can think of which does bite us is this: #define FOO(x) if (x < 1 || x > 3) FOO(x++) I don't think people will be confused by NS_DEBUG_ASSIGN.

Yuck. I really don't like this solution, for a couple of reasons: 1) Adding strange macros makes the Mozilla code less approachable, especially at a time when we're trying to make it _more_ approachable. 2) Some of these patches seem like they're just wallpapering over the compiler warning instead of fixing the problem (say, by passing on the error or having the callee return void and assert internally). For example, in bug 577908 you basically have: NS_DEBUG_ASSIGN(ok) doSomething(); NS_ASSERTION(ok, "oops we failed") return NS_OK;

How about #ifdef DEBUG #define NS_DEBUG_ONLY(x) x #else #define NS_DEBUG_ONLY(x) #endif

Comment on attachment 456813 [details] [diff] [review] assign or void Yeah, I don't think this macro is intelligible. I don't mind NS_DEBUG_ONLY if necessary.

Always give the macro a body, ((void)0) works, to avoid empty statement warnings from some compilers. /be

so... NS_DEBUG_ONLY would look like this: #define NS_DEBUG_ONLY(lhs) lhs or #define NS_DEBUG_ONLY(lhs) (void) NS_DEBUG_ONLY(ok =) doSomething();

No, that's not what I wrote, and people already barfed on macros whose bodies are not well-formed expressions or statements (dangling-else immune statements, see {PR,JS}_{BEGIN,END}_MACRO). What Neil wrote with the fix I suggested looks like this: #ifdef DEBUG #define NS_DEBUG_ONLY(x) x #else #define NS_DEBUG_ONLY(x) ((void)0) #endif /be

that's not helpful. the problem case is that the assignment is only used in debug builds. the right hand side needs to be evaluated.

(In reply to comment #16) > that's not helpful. See below. At issue is the non-syntactic macro being unsafe and unhelpful on balance. > the problem case is that the assignment is only used in debug builds. the right > hand side needs to be evaluated. I know -- I think (and thought others here agreed) that you should #ifdef DEBUG such declaration and left-hand side followed by =. This is what we do in SpiderMonkey. It's better to avoid non-syntactic magic macros when doing anything like this. Take the #ifdef pain, call the reader's attention to special case. Example from SpiderMonkey: #ifdef DEBUG PropertyCacheEntry *entry = #endif JS_PROPERTY_CACHE(cx).fill(cx, scopeChain, scopeIndex, protoIndex, pobj, (JSScopeProperty *) prop); JS_ASSERT(entry); /be

If we are going to change the code as proposed in attachment 456746 [details] [diff] [review], I'll have to get a C preprocessor brain implant. I'd prefer the style shown in comment 17.

OK if NS_DEBUG_DECL doesn't fly, how about a variant of NS_ASSERTION, say NS_CHECK, where the condition is evaluated in non-DEBUG builds? (But nothing special happens if it fails.)

Not reopening yet, but I really liked Jonathon's suggestion: #define NS_UNUSED_IF_RELEASE __attribute__((unused)) But nobody ever mentioned it again. It seems quite clean to me.

There are three problems here, I think (1) Silence warnings for locals used only in debug builds (2) Don't allocate stack space for debug-only locals (3) Don't evaluate rhs's in assignment or initialization of debug-only locals if the rhs computation is unnecessary The last item is the opposite in a sense of the example in comment 17. AFAIK, the only way to guarantee all three is macro-ization, but I don't think anyone is too enthused about that. We can get pretty close with a template system like template<typename T> struct DebugOnly { #ifdef DEBUG /* ... */; T v; #else /* ... */ #endif }; in which - both variants of DebugOnly implement default-ctor, copy-ctor, operator= - non-DEBUG variant impls are no-ops - only the DEBUG impl exposes type-coercion operators etc., to access stored value This gets us (1), (2), and part of the way to (3). For an expression like DebugOnly<bool> a = ExpensiveSideEffecty(); the compiler won't be able to boil away the call. Simpler expressions could be boiled away. Maybe this isn't a bad thing. Another sort-of advantage to this scheme is somewhat readable errors from the compiler if debug-only locals are used in opt builds. For example bool nd = ExpensiveSideEffecty(); DebugOnly<bool> db = ExpensiveSideEffecty(); ASSERT(nd == db); // ^^^^ always OK if (nd == db) { /*...*/ } // In non-DEBUG builds, gives // error: no match for ‘operator==’ in ‘nd == db’ nd = db; // In non-DEBUG builds, gives // error: cannot convert ‘DebugOnly<bool>’ to ‘bool’ in assignment Thoughts?

(In reply to comment #22) > This gets us (1) Um oops, not this; forgot my -Wall. We'd need some magic attribute for DebugOnly instances.

Mmmm, I like comment 22. Just in case, I verified that gcc is actually able to avoid wasting stack space for empty structs and (at least for my simple recursive example), it seems to have no problem.

(In reply to comment #21) > Not reopening yet, but I really liked Jonathon's suggestion: > #define NS_UNUSED_IF_RELEASE __attribute__((unused)) > > But nobody ever mentioned it again. It seems quite clean to me. Yeah, this seems reasonable to me. Except I'm worried that people might use it when they really mean for the entire statement rather than just the assignment to be #ifdef DEBUG. I'm not crazy about using five lines of height: #ifdef DEBUG nsresult rv = #endif do_something(); NS_ASSERTION(NS_SUCCEEDED(rv), "unexpected failure"); where it seems like two lines ought to do, but maybe it's the best solution. Then again, I think my review- on the layout patch that did that, on grounds of being way too ugly, might have been what prompted this in the first place...

Actually, my suggestion (bug 577914 comment 4) was after this was filed, was never mentioned here, and I review+'d the patch (not review-). I'd suggested a macro that would give: NS_IF_DEBUG(nsIContent* propagatedScrollFrom =) PropagateScrollToViewport(); which I think is a little less magical than timeless's proposals. But plain old #ifdef DEBUG might still be better.

I'll take a stab at a DebugOnly template after some higher-priority bugs.

dbaron: re comment 26, i think that's my offer in comment 14 which brendan didn't like in comment 15.

Tentative rules - non-standard files added to mfbt are named LikeThis.h, because that's the style SM and Gecko have converged on - non-standard code added to mfbt is in the "mozilla" namespace - coding style follows SM, because Gecko coding style is ugly ;) - folks will probably know who's best to review code going into mfbt, but a module could be created if necessary It's not 100% clear how mfbt files will be distributed with SM source releases, but that doesn't seem like a difficult problem. Please let me know if I've missed folks who should review this.

There are already bugs filed for some of these fixes, but I figured I'd include this for demonstration purposes.

Comment on attachment 519537 [details] [diff] [review] part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko Guess I can't tag more than one sr.

Comment on attachment 519538 [details] [diff] [review] part 2: Add a DebugOnly helper to mfbt, which only contains a value in debug builds Glad to see this idea landing!

Comment on attachment 519539 [details] [diff] [review] Example uses of DebugOnly If you want to spread your idea further and get the build-plumbing working for using mozilla/mfbt in js/src, :vim /#ifdef DEBUG.*\n.* = *$/ * points to 12 sites that are begging to be beautified.

Comment on attachment 519538 [details] [diff] [review] part 2: Add a DebugOnly helper to mfbt, which only contains a value in debug builds >+ * DebugOnly instances can only be coerced to T in debug builds; in >+ * release builds, then don't have a value so type coercion is not >+ * well defined. I think this wants s/then/they/, right? (in "then don't have a value")

Definitely! We've also got what looks to be 20 or so unused variable warnings in gecko that I'm looking to zap once this can land.

(In reply to comment #35) > I think this wants s/then/they/, right? (in "then don't have a value") Oops yep, thanks.

Comment on attachment 519538 [details] [diff] [review] part 2: Add a DebugOnly helper to mfbt, which only contains a value in debug builds We should probably also move the static analysis annotations to a shared location: this class should be NS_STACK_CLASS. Note that an empty class as a member will still have a sizeof(1) because that is necessary in order to satisfy C++ requirements about taking its address. In release builds I believe that it will be optimized away as a local variable, but we should probably check that.

Comment on attachment 519537 [details] [diff] [review] part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko How is this going to work in the standalone JS build? It looks to me like this ought to be js/src/mfbt and ought to be built as part of spidermonkey.

Comment on attachment 519537 [details] [diff] [review] part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko ># HG changeset patch ># User Chris Jones <jones.chris.g@gmail.com> ># Date 1300229392 18000 ># Node ID a6be83c442390b44a9b90a2501b092dfdeaf7163 ># Parent 4866be78732f042ba9ffed96fc69a40942d16e26 >Bug 577899, part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko. r=luke,ted sr=brendan,bsmedberg,roc > >diff --git a/config/js/build.mk b/config/js/build.mk >--- a/config/js/build.mk >+++ b/config/js/build.mk >@@ -31,9 +31,9 @@ > # decision by deleting the provisions above and replace them with the notice > # and other provisions required by the GPL or the LGPL. If you do not delete > # the provisions above, a recipient may use your version of this file under > # the terms of any one of the MPL, the GPL or the LGPL. > # > # ***** END LICENSE BLOCK ***** > > TIERS += js >-tier_js_dirs = js/src >+tier_js_dirs = js/src mfbt This builds mfbt after js/src. Is that really what you want? >diff --git a/mfbt/Makefile.in b/mfbt/Makefile.in >new file mode 100644 >--- /dev/null >+++ b/mfbt/Makefile.in >+include $(topsrcdir)/config/config.mk >+include $(topsrcdir)/config/rules.mk You don't need to explicitly include config.mk (rules.mk does so).

(In reply to comment #38) > Comment on attachment 519538 [details] [diff] [review] > part 2: Add a DebugOnly helper to mfbt, which only contains a value in debug > builds > > We should probably also move the static analysis annotations to a shared > location: this class should be NS_STACK_CLASS. Yes, good idea. Centralizing the annotations would be biting off a fair amount more work, the foundation for which is being laid in bug 642381. We could block landing DebugOnly on that, but I would be a bit unhappy about that because DebugOnly itself doesn't depend on the deeper problems being worked out in 642381, and not landing DebugOnly will keep going the steady influx of ugly #ifdef DEBUG nsresult rv = #endif patches. As a compromise, how does adding a dummy #define NS_STACK_CLASS to Util.h plus the "real" struct NS_STACK_CLASS DebugOnly { suit? > In release builds I believe > that it will be optimized away as a local variable, but we should probably > check that. Yes, I checked that. Judging by comment 24, I believe Luke did too.

(In reply to comment #39) > Comment on attachment 519537 [details] [diff] [review] > part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko > > How is this going to work in the standalone JS build? It looks to me like this > ought to be js/src/mfbt and ought to be built as part of spidermonkey. This is being worked out in bug 642381. Parts 1 and 2 here are dancing around the build problems by having mfbt/ only contain headers. This is *not* avoiding the distribution problem, but I haven't heard that one is imminent. Basically, the patches here are deferring all the hard build work to bug 642381, and trying to get in a useful helper (to avoid more ugly DEBUG patches) while that stuff is being sorted out.

(In reply to comment #40) > Comment on attachment 519537 [details] [diff] [review] > > TIERS += js > >-tier_js_dirs = js/src > >+tier_js_dirs = js/src mfbt > > This builds mfbt after js/src. Is that really what you want? For this bug it doesn't matter, but I found out in bug 642381 that, no, that is indeed not what I want :). Thanks. > You don't need to explicitly include config.mk (rules.mk does so). Removed.

Comment on attachment 519537 [details] [diff] [review] part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko Not much to see here -- what am I missing. /be

Reply/review ping.

Comment on attachment 519537 [details] [diff] [review] part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko From in-person conversation, this should be built from within js/src, so that standalone JS can use it (from a build-ordering perspective). I would vaguely prefer all the files to live in js/mfbt, but I don't have a really strong opinion about it.

Moved all build goop into js/src/Makefile.in. Much cleaner, thanks!

Comment on attachment 522917 [details] [diff] [review] part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko, v2 in js/src, $(topsrcdir)/mfbt is js/src/mfbt. Does this patch actually work?

It was working locally because I had an old copy of Util.h in dist/include from the previous version of the patch. I accidentally pushed this to try and saw that it was busted for clobbers. The fix was just +VPATH += \ + $(srcdir)/../../mfbt \ + $(NULL) so I didn't think it was important enough to re-post.

Comment on attachment 522917 [details] [diff] [review] part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko, v2 r=me the right way, then!

http://hg.mozilla.org/mozilla-central/rev/91a8d742c509 http://hg.mozilla.org/mozilla-central/rev/bfef135a83dc

Added https://developer.mozilla.org/index.php?title=En/Namespace/Mozilla/DebugOnly%3CT%3E . Please let me know if something is unclear.

Comment on attachment 522917 [details] [diff] [review] part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko, v2 >diff --git a/mfbt/Util.h b/mfbt/Util.h >new file mode 100644 >--- /dev/null >+++ b/mfbt/Util.h >@@ -0,0 +1,47 @@ >+ * Portions created by the Initial Developer are Copyrigght (C) 2011 >+ * the Initial Developer. All Rights Reserved. Copy-what?

I cleaned up the doc cjones wrote and added it to the Firefox 5 for developers page. Also added a note about it to: https://developer.mozilla.org/En/Developer_Guide/Coding_Style#C.2fC.2b.2b_Practices