Last Comment Bug 647011 - Convert #ifdef DEBUG variables into DebugOnly<T>
: Convert #ifdef DEBUG variables into DebugOnly<T>
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 577899
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-31 15:17 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2011-04-27 16:51 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Sprinkle some DebugOnly in js/src (13.64 KB, patch)
2011-03-31 19:17 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Sprinkle some DebugOnly in js/src (12.45 KB, patch)
2011-04-07 13:43 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
luke: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-31 15:17:02 PDT
Mechanical rewrite of

#ifdef DEBUG
  T foo =
#endif
  Bar();

to

  DebugOnly<T> foo = Bar();
Comment 1 Luke Wagner [:luke] 2011-03-31 15:18:35 PDT
yeeehaaaw
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-31 16:27:12 PDT
$ find -type f -name 'js*' -exec mgrep -l '#ifdef DEBUG[^\n]*\n\s*\S+\s+\S+\s*=' {} \; | wc -l
27
$ find -type f -name 'js*' -exec mgrep -l '#ifdef DEBUG[^\n]*\n\s*\S+\s+\S+\s*=' {} \;
./jsstrinlines.h
./jsfun.h
./jsscan.cpp
./jsobj.cpp
./jsinterp.cpp
./jsapi.h
./jscntxtinlines.h
./jsscript.cpp
./jstracer.cpp
./jsatom.cpp
./jscntxt.h
./jsstr.cpp
./jsapi.cpp
./jspropertytree.cpp
./jscntxt.cpp
./jsgc.cpp
./jsscope.cpp
./shell/js.cpp
./jsemit.cpp
./jsarray.cpp
./jspropertycache.cpp
./jsobjinlines.h
./jsparse.cpp
./jsinterpinlines.h
./jsxml.cpp
./jstl.h
./jsfun.cpp
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-31 16:37:47 PDT
By which I meant

$ find -type f -name 'js*' -exec mgrep -l '\s*#ifdef DEBUG\s*\n\s*\S+\s+\S+\s*=\s*\n\s*#endif' {} \; 
./jsobj.cpp
./jsscript.cpp
./jstracer.cpp
./jscntxt.h
./jscntxt.cpp
./shell/js.cpp
./jsemit.cpp
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-31 19:17:03 PDT
I ended settling on

$ find -type f -name 'js*' -exec mgrep -l '#ifdef DEBUG[^\n]*\n\s*\S+\s+\S[^=]*=\s*\n\s*#endif' {} \; 

The files I left unfixed in js/src were in xpconnect and the following
./methodjit/FastOps.cpp
./methodjit/StubCalls.cpp
./yarr/yarr/RegexCompiler.cpp
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-31 19:17:53 PDT
Created attachment 523499 [details] [diff] [review]
Sprinkle some DebugOnly in js/src
Comment 6 Luke Wagner [:luke] 2011-04-01 20:02:38 PDT
Sorry for the delay.  We could decrease the syntactic distinction between mozilla/mfbt and js/mfbt with:

// js/src/jsutil.h
#include "mozilla/Util.h"
namespace js { using namespace mozilla; }

Then, we could use "js::DebugOnly<T>" in headers and "DebugOnly<T>" in .cpps (where we already have 'using namespace js').

Is that reasonable or too weird?
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-01 20:15:52 PDT
No hurry.

Suits me, except for potential name collisions (bug 642381 comment 11).  I'd be fine dealing with that if/when it arises though.
Comment 8 Luke Wagner [:luke] 2011-04-01 21:29:08 PDT
(In reply to comment #7)
> Suits me, except for potential name collisions (bug 642381 comment 11).  I'd be
> fine dealing with that if/when it arises though.

Ah, I missed that comment before.  Since I'm thinking that js/mfbt is going to be js::Hash{Set,Map}, js::Vector and not much else, I'm not too worried; we should be able to keep them coherent.  (Btw, bug 632137 is indeed making its way to the front of my work queue :)
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-06 09:47:32 PDT
Sounds good.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-07 13:43:15 PDT
Created attachment 524472 [details] [diff] [review]
Sprinkle some DebugOnly in js/src
Comment 11 Luke Wagner [:luke] 2011-04-07 14:07:40 PDT
Comment on attachment 524472 [details] [diff] [review]
Sprinkle some DebugOnly in js/src

Rock on
Comment 12 Chris Leary [:cdleary] (not checking bugmail) 2011-04-26 15:31:01 PDT
cjones, 'tis customary to post the tracemonkey changeset URI when you mark as fixed-in-tracemonkey.

http://hg.mozilla.org/mozilla-central/rev/fa2c397985a2
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-27 16:51:53 PDT
Urgh, sorry, I knew that but forgot.

http://hg.mozilla.org/tracemonkey/rev/fa2c397985a2

Note You need to log in before you can comment on or make changes to this bug.