647011 - Convert #ifdef DEBUG variables into DebugOnly<T>

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Mechanical rewrite of

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

to

  DebugOnly<T> foo = Bar();

Comment 1

[Luke Wagner [:luke]]

yeeehaaaw

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

$ 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]

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]

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]

Attachment: Sprinkle some DebugOnly in js/src

Comment 6

[Luke Wagner [:luke]]

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]

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]]

(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]

Sounds good.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Sprinkle some DebugOnly in js/src

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 524472 [details] [diff] [review]
Sprinkle some DebugOnly in js/src

Rock on

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

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]

Urgh, sorry, I knew that but forgot.

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