The default bug view has changed. See this FAQ.

Convert #ifdef DEBUG variables into DebugOnly<T>

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

Mechanical rewrite of

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

to

  DebugOnly<T> foo = Bar();

Comment 1

6 years ago
yeeehaaaw
$ 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
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
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
Created attachment 523499 [details] [diff] [review]
Sprinkle some DebugOnly in js/src
Assignee: general → jones.chris.g
Attachment #523499 - Flags: review?(luke)

Comment 6

6 years ago
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?
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

6 years ago
(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 :)
Sounds good.
Created attachment 524472 [details] [diff] [review]
Sprinkle some DebugOnly in js/src
Attachment #523499 - Attachment is obsolete: true
Attachment #523499 - Flags: review?(luke)
Attachment #524472 - Flags: review?(luke)

Comment 11

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

Rock on
Attachment #524472 - Flags: review?(luke) → review+
Whiteboard: fixed-in-tracemonkey
cjones, 'tis customary to post the tracemonkey changeset URI when you mark as fixed-in-tracemonkey.

http://hg.mozilla.org/mozilla-central/rev/fa2c397985a2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Urgh, sorry, I knew that but forgot.

http://hg.mozilla.org/tracemonkey/rev/fa2c397985a2
You need to log in before you can comment on or make changes to this bug.