The default bug view has changed. See this FAQ.

Assignment to a DebugOnly variable is automatically elided in non-debug builds.

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
--
trivial
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla9
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
Attachment #556399 - Flags: review?(bhackett1024)

Comment 1

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #556399 - Flags: review?(bhackett1024)
(Assignee)

Comment 2

6 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
Attachment #556399 - Flags: review+

Comment 3

6 years ago
Oops, I totally misread comment 0 and failed to look at the patch.  Sorry for the noise.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
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! :-)
Assignee: general → terrence
Status: REOPENED → ASSIGNED
Flags: in-testsuite-
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Version: Other Branch → Trunk
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
}
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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.
Attachment #556399 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Has this been reviewed/looked at again since the changes?
http://hg.mozilla.org/integration/mozilla-inbound/rev/a0a6800d3744
Keywords: checkin-needed
Target Milestone: --- → mozilla9
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.
(Assignee)

Comment 11

6 years ago
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 :-).
Target Milestone: mozilla9 → ---
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 :-)
Target Milestone: --- → mozilla9
(Assignee)

Comment 13

6 years ago
Gah!!!  Posted a notice on 646597.  My error rate is way too high.  I'm making a checklist.
Thanks :-)

http://hg.mozilla.org/mozilla-central/rev/a0a6800d3744
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.