Last Comment Bug 682717 - Assignment to a DebugOnly variable is automatically elided in non-debug builds.
: Assignment to a DebugOnly variable is automatically elided in non-debug builds.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla9
Assigned To: Terrence Cole [:terrence]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-28 09:59 PDT by Terrence Cole [:terrence]
Modified: 2011-09-04 16:45 PDT (History)
5 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove unnecessary #ifdef DEBUG around assignment to DebugOnly. (1.25 KB, patch)
2011-08-28 09:59 PDT, Terrence Cole [:terrence]
bhackett1024: review+
Details | Diff | Splinter Review
Remove unneeded #ifdef DEBUG: use DebugOnly instead. (1.88 KB, patch)
2011-09-02 11:31 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2011-08-28 09:59:16 PDT
Created attachment 556399 [details] [diff] [review]
Remove unnecessary #ifdef DEBUG around assignment to DebugOnly.

On jaegermonkey tip in jsanalyze.cpp ScryptAnalysis::analyzeLifetimes on line 908, assignment to the DebugOnly<bool> variable found is protected by #ifdef DEBUG.

Assignment to a DebugOnly variable results in a call to either the empty 'operator =' or to an empty implicit 1-arg constructor and empty copy constructor when in non-debug mode.  Either of these get inlined and eliminated as dead code above -00.  No code is produced for the statement, so there is no benefit to cluttering the code with this condition.
Comment 1 Luke Wagner [:luke] 2011-08-28 17:41:30 PDT
That may be the case for the specific example you looked at, but can you promise me that will happen on both GCC and MSVC for all possible real uses for modern and future versions?  Answering those with an unequivocal "yes" costs a few #ifdefs in reusable code that gets read maybe once per developer.  I'd rather keep things obvious.
Comment 2 Terrence Cole [:terrence] 2011-08-28 22:15:25 PDT
Ah, I thought that was half the point of DebugOnly.  This makes me sad.

In this case, I have to admit that the version with more #ifdef's is indeed the clearer one.  Sorry for the noise.
Comment 3 Luke Wagner [:luke] 2011-08-29 13:40:26 PDT
Oops, I totally misread comment 0 and failed to look at the patch.  Sorry for the noise.
Comment 4 Ed Morley [:emorley] 2011-08-30 11:35:49 PDT
In my queue :-)

I've changed the commit message slightly, since there was no bug number and no r= for the reviewer. Also, the pushlog + TBPL only display the first line/80 characters, so a summary in the first line makes it a bit clearer. eg:
"Bug 682717 - Remove unnecessary #ifdef DEBUG around assignment to DebugOnly; r=bhackett"

I've left the more detailed commit message after that - since it does a great job of making the changeset clearer later on, without having to return to the bug - and is something that people don't do often enough, so thanks! :-)
Comment 5 Ed Morley [:emorley] 2011-08-30 11:58:29 PDT
This fails my MSVC2010 local build:

{
jsanalyze.cpp

c:\mozilla\repos\inbound\js\src\methodjit/MethodJIT.h(690) : warning C4305: 'argument' : truncation from 'js::MaybeConstruct' to 'bool'

c:\mozilla\repos\obj-inbound\dist\include\mozilla\util.h(177) : error C4716: 'mozilla::DebugOnly<bool>::operator=' : must return a value


In the directory  /c/mozilla/repos/obj-inbound/js/src
The following command failed to execute properly:
c:/mozilla-build/python/python2.7.exe -O c:/mozilla/repos/inbound/js/src/build/cl.py cl -Fojsanalyze.obj -c -DOSTYPE="WINNT6.1" -DOSARCH=WINNT -DEXPORT_JS_API -DIMPL_MFBT -DJS_HAS_CTYPES -DDLL_PREFIX="" -DDLL_SUFFIX=".dll" -Ictypes/libffi/include -I. -I../../../inbound/js/src -I. -I./../../dist/include -I./../../dist/include/nsprpub -Ic:/mozilla/repos/obj-inbound/dist/include/nspr -I../../../inbound/js/src -I../../../inbound/js/src/assembler -I../../../inbound/js/src/yarr -GR- -w14505 -TP -nologo -wd4345 -D_CRT_SECURE_NO_WARNINGS -W3 -Gy -Fdgenerated.pdb -wd4244 -wd4800 -we4553 -DNDEBUG -DTRIMMED -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JIT=1 -MD -FI ./js-confdefs.h -DMOZILLA_CLIENT c:/mozilla/repos/obj-inbound/js/src/../../../inbound/js/src/jsanalyze.cpp
}
Comment 6 Terrence Cole [:terrence] 2011-08-30 12:28:42 PDT
Huh.  I assumed that because this operator= was already present in the tree I would not be the first person using it.  I will look into making MSVC happy.
Comment 7 Terrence Cole [:terrence] 2011-09-02 11:31:02 PDT
Created attachment 557893 [details] [diff] [review]
Remove unneeded #ifdef DEBUG: use DebugOnly instead.

Tested in linux and windows to ensure that this does correctly inline, with the new operator= body.  Tested in linux with make check on js/src on an optimized build and a full compile of mozilla-central.
Comment 8 Ed Morley [:emorley] 2011-09-04 02:52:38 PDT
Has this been reviewed/looked at again since the changes?
Comment 10 Ed Morley [:emorley] 2011-09-04 07:09:52 PDT
Meant to add that comment 8 didn't seem necessary looking at the changes + it built locally and on try fine, so pushed to inbound.
Comment 11 Terrence Cole [:terrence] 2011-09-04 09:11:55 PDT
There was no second review -- Luke and I discussed the necessary change in IRC.  As you noticed, it should be small enough to fly under the radar.  Sorry I missed such an obvious problem on the first pass.  Once I get access to a try server, I think your workload will drop dramatically :-).
Comment 12 Ed Morley [:emorley] 2011-09-04 10:40:41 PDT
Yeah after comment 8 I remembered you discussing it on #jsapi, so thought you had probably got as rs+ over IRC anyway.

I've just spotted that the bug number in the commit message was actually for one of the other bugs you are working on. Could you post a reply in the other bug pointing them this way please (both to help the person merging and for someone looking at hg annotate later). Thanks :-)
Comment 13 Terrence Cole [:terrence] 2011-09-04 12:43:31 PDT
Gah!!!  Posted a notice on 646597.  My error rate is way too high.  I'm making a checklist.
Comment 14 Ed Morley [:emorley] 2011-09-04 16:45:45 PDT
Thanks :-)

http://hg.mozilla.org/mozilla-central/rev/a0a6800d3744

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