The default bug view has changed. See this FAQ.

Remove NULL-check before delete

RESOLVED FIXED in mozilla7

Status

()

Core
General
--
trivial
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Hans-Andreas Engel, Assigned: emorley)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

12 years ago
Because |delete| checks if its argument is null, we can replace |if (a) delete a;| by |delete a;|.
(Reporter)

Comment 1

12 years ago
Approximately 60 .cpp files are affected.
(Reporter)

Comment 2

12 years ago
I stumbled over this when I wanted to use |if (a) delete a;| but David pointed out that |delete| already checks for null (Bug 315975 Comment 12).
(Reporter)

Comment 3

12 years ago
Basically, this change can be implemented by running

find . -name \*.cpp | perl -0 -e '@ARGV = split(/\s+/,<>);  while(<>){ '      \
 -e 'if(s/if\s*\(\s*([_a-zA-Z0-9]+)\s*\)\s*delete\s+\1[ \t]*/delete $1/gs  )' \
 -e '{ print "$ARGV\n"; open OUT,">",$ARGV or die "cannot write to $ARGV\n";' \
 -e 'print OUT;} }' | tee changed_files
cvs diff -up8N `cat changed_files`
(Reporter)

Comment 4

12 years ago
Created attachment 203220 [details] [diff] [review]
Globally replace |if(a) delete a;| by |delete a;|
(Reporter)

Updated

12 years ago
Attachment #203220 - Flags: review?(bienvenu)

Comment 5

12 years ago
Comment on attachment 203220 [details] [diff] [review]
Globally replace |if(a) delete a;| by |delete a;|

watch out for tabs (also in most of the mailnews files and nsStorageStream.cpp)

>RCS file: /cvsroot/mozilla/gfx/src/photon/nsRenderingContextPh.cpp,v
>-	if( mTranMatrix ) 
>-		delete mTranMatrix;
>+  delete mTranMatrix;
(Reporter)

Comment 6

12 years ago
In an extra effort, I removed the tabs the modified lines and replaced them according to the emacs-parameters of the corresponding .cpp-file.

For example, |gfx/src/photon/nsRenderingContextPh.cpp| defines
   Mode: C++; tab-width: 2; indent-tabs-mode: nil;  (...)
thus, I used 2 spaces for each tab and replaced |\tdelete mTranMatrix;| by |  delete mTranMatrix;|.  Indeed, the patch file might look somewhat weird when opened in mozilla, because mozilla typically displays tabs as 6 spaces.

I thought that new code lines should not contain any tabs, such that all tabs are gradually removed from the code.  Please correct me if I am wrong; I would be happy to produce a version with tabs (this is much easier to automatically generate anyway).

Comment 7

12 years ago
> I thought that new code lines should not contain any tabs, such that all tabs
> are gradually removed from the code.

Correct. Shoot 'em where ye stumble upon 'em!
(Reporter)

Comment 8

12 years ago
On the "Mozilla Coding Style Guide,"  
   http://www.mozilla.org/hacking/mozilla-style-guide.html
I found a link to "JST Review Simulacrum,"
   http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl
which lists the error "W - (...) tabs on the line (...)."

Thus, the Florida rule for tabs (shoot 'em, see comment above) also seems to apply there.

Comment 9

12 years ago
also in http://www.mozilla.org/hacking/mozilla-style-guide.html

> Whitespace: No tabs. No whitespace at the end of a line.

but right before that is:
> Use the prevailing style in a file or module, or ask the owner, if you are on 
> someone else's turf. Module owner rules all.

Practically speaking, most of those files could be converted to no-tabs and no one would complain, but if you only convert one line then it looks bad and is hard to read.
(Reporter)

Updated

12 years ago
Attachment #203220 - Attachment is patch: false
Attachment #203220 - Flags: review?(bienvenu)
(Reporter)

Updated

12 years ago
Attachment #203220 - Attachment is obsolete: true
Attachment #203220 - Attachment is patch: true
(Reporter)

Comment 10

12 years ago
Adrew, thank you very much from clarifying the tab issue and thereby saving me from the wrath of the file/module owners.

The only file where I should remove tabs is xpcom/io/nsStorageStream.cpp, and indeed I should keep the tabs in all other files.
(Reporter)

Comment 11

12 years ago
Created attachment 203286 [details] [diff] [review]
Globally replace |if(a) delete a;| by |delete a;|, keep '\t's
(Reporter)

Updated

12 years ago
Attachment #203286 - Flags: review?(bienvenu)
bienvenu, are you still best person to review?
Hans-Andreas is "currently not active in the mozilla projects"
Assignee: engel → nobody
Whiteboard: [patchlove][has patch][needs owner]

Comment 14

8 years ago
Comment on attachment 203286 [details] [diff] [review]
Globally replace |if(a) delete a;| by |delete a;|, keep '\t's

many of the mailnews ones were already fixed; I can't review the mozilla ones
Attachment #203286 - Attachment is obsolete: true
Attachment #203286 - Flags: review?(bienvenu)
The existing patch had bitrotted significantly, so am starting from scratch. The perl/regex incantations in comment 3 proved invaluable, thanks!

However, it only covers:
|if (foo) delete foo;|
...and not 
|if (foo) { delete foo; }|

The following covers the second case (only; I ran after the comment 3 version to cover both cases):

find . -name \*.cpp | perl -0 -e '@ARGV = split(/\s+/,<>);  while(<>){ '      \
 -e 'if(s/if\s*\(\s*([_a-zA-Z0-9]+)\s*\)\s*\{\s*delete\s+\1(.).*?\}[ \t]*/delete $1\2/gs  )' \
 -e '{ print "$ARGV\n"; open OUT,">",$ARGV or die "cannot write to $ARGV\n";' \
 -e 'print OUT;} }'

This new regex also catches anything else that is inside the { delete foo; } block (which I didn't imagine would contain anything else, hence simplifying the regex) - but in fact there are several instances of:
> if (foo) {
>   delete ocm;
>   foo = nsnull;
> }

eg: http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/src/nsOperaProfileMigrator.cpp#630
and
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFontconfigUtils.cpp#63
...plus many more.

Isn't the |foo = nsnull| redundant? (And so safe to be removed). Or am I misunderstanding what delete does and so these cases need to be left as is?

Thanks!
Assignee: nobody → bmo
Status: NEW → ASSIGNED
s/ocm/foo

Comment 17

6 years ago
(In reply to comment #15)

> Isn't the |foo = nsnull| redundant? (And so safe to be removed). Or am I
> misunderstanding what delete does and so these cases need to be left as is?
thx for continuing this work. No, foo = null; is not redundant; nulling out the pointer is the way a lot of code knows that it needs to re-allocate the pointer. Otherwise, we could end up using freed memory, which leads to lots of crashes and potential security exploits.
Thanks for the clarification, I'm kind of learning c/c++ as I go here (something that comment 15 no doubt made apparent!) and I thought delete might possibly be nulling out the pointer at the same time, which it's obviously not.

For these cases then:

> if (foo) {
>   delete ocm;
>   foo = nsnull;
> }

Should they be left verbatim, or changed to:

> delete ocm;
> foo = nsnull;

I'm presuming the former? (Since the potentially unnecessary null check is outweighed by the chance of doing unnecessary |foo = nsnull|s ?)

Comment 19

6 years ago
I think the example would be

if (foo) {
  delete foo;
  foo = nsnull
}

I assume we're talking about foo the whole time;

I don't particularly care which way we go - probably the former is more efficient, though the latter makes the code easier to read.
Sorry bad copy and paste from my original typo in comment 15, yeah foo all the time.

Again, at the risk of sounding stupid (but I'd rather learn why, so I'm going to say this anyway), but why aren't all the instances of delete followed by nulling out the pointer? 

My first thought was that for cases where the variable is never used again, it was not deemed necessary - but then there are places like this:
http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/src/nsOperaProfileMigrator.cpp#630

...which still nulls out the pointer even though the next line is a return, and the variable isn't used again...?

Comment 21

6 years ago
you're right, that null out is not needed.
So would you like me to remove it (and any others that are not needed either) in this patch as well?
Ok, on the advice of Hans-Andreas Engel, I will remove ones I can, but I'll separate out into a second patch for easier review.

The regex above missed a few cases (eg arrays, since the [i] didn't match the [_a-zA-Z0-9]+ ), so I've revised them slightly...

1st pass: (for the |if (foo) delete foo;| case)
s/if[ \t]*\([ \t]*([_a-zA-Z0-9\[\]]+)[ \t]*\)\s*delete[ \t]+\1/delete $1/gs

2nd pass: (for the |if (foo) { delete foo; }| case)
s/if[ \t]*\([ \t]*([_a-zA-Z0-9\[\]]+)[ \t]*\)\s*\{\s*delete[ \t]+\1;\s*\}/delete $1;/gs

After manually reviewing the output, I reverted a false positive to an if-else delete block in xpcom/ds/nsCheapSets.cpp. A double check using regex that deliberately catches the whole if {} block, even if it contains other lines, showed up a few with comments that were safe to include as well.

There were changes made to files in hunspell and ANGLE; but had to revert given from upstream. Do you think it's worth filing bugs for them upstream?
Component: XPCOM → General
QA Contact: xpcom → general
Whiteboard: [patchlove][has patch][needs owner] → [patchlove][has patch]
Created attachment 530910 [details] [diff] [review]
Part A: For mozilla-central

Remove null check before delete in everything other than js/; for landing on m-c.
Created attachment 530911 [details] [diff] [review]
Part B: Changes to js/, for landing on TM

Remove null check before delete from js/*; for landing on tracemonkey.
Created attachment 530914 [details] [diff] [review]
Part C: Unnecessary null outs; for m-c

Remove unnecessary null outs from:
- browser/components/migration/src/nsOperaProfileMigrator.cpp
http://mxr.mozilla.org/mozilla-central/search?string=ocm&find=/browser/components/migration/
- xpcom/build/nsXPComInit.cpp
http://mxr.mozilla.org/mozilla-central/search?string=sIOThread&find=/xpcom/
Parts A, B & C:
http://dev.philringnalda.com/tbpl/?tree=Try&rev=f88432556f2e
Whiteboard: [patchlove][has patch]
Comment on attachment 530910 [details] [diff] [review]
Part A: For mozilla-central

>diff --git a/layout/mathml/nsMathMLChar.cpp b/layout/mathml/nsMathMLChar.cpp
>--- a/layout/mathml/nsMathMLChar.cpp
>+++ b/layout/mathml/nsMathMLChar.cpp
>@@ -1767,17 +1767,17 @@ nsMathMLChar::ComposeChildren(nsPresCont
>   nsMathMLChar* last = this;
>   while ((i < count) && last->mSibling) {
>     i++;
>     last = last->mSibling;
>   }
>   while (i < count) {
>     child = new nsMathMLChar(this);
>     if (!child) {
>-      if (mSibling) delete mSibling; // don't leave a dangling list ...
>+      delete mSibling; // don't leave a dangling list ...
>       mSibling = nsnull;
>       return NS_ERROR_OUT_OF_MEMORY;
>     }

new can't fail, so please remove the entire |if (!child)| block.
Created attachment 530960 [details] [diff] [review]
Part A: For mozilla-central

Part A updated for comment 28 feedback.

Just going to have to pick random people to review these, since not really sure who best for multi-component changes.
Attachment #530910 - Attachment is obsolete: true
Attachment #530960 - Flags: review?(benjamin)
Attachment #530911 - Flags: review?(pbiggar)
Attachment #530914 - Flags: review?(shaver)

Comment 30

6 years ago
Comment on attachment 530911 [details] [diff] [review]
Part B: Changes to js/, for landing on TM

Review of attachment 530911 [details] [diff] [review]:
-----------------------------------------------------------------

We don't actually own half this stuff, so this r+ is only for jsweakmap.cpp.

(See https://wiki.mozilla.org/Modules for hints about reviewers - if you're lucky it'll be in date ;). I'm not sure if jsd uses the tm tree, so ask that too.)

::: js/jsd/jsd_xpc.cpp
@@ -1020,5 @@
>  {
>      DEBUG_DESTROY ("jsdScript", gScriptCount);
> -    if (mFileName)
> -        delete mFileName;
> -    if (mFunctionName)

JS team owns js/src, not js/jsd. Try brendan or timeless.

::: js/src/jsweakmap.cpp
@@ -296,5 @@
>  WeakMap::finalize(JSContext *cx, JSObject *obj)
>  {
>      WeakMap *table = fromJSObject(obj);
> -    if (table)
> -        delete table;

I thought from reading this that fromJSObject might be fallible in some way and that the check had some semantic meaning, but it doesn't look like it. So yes, remove this check.

::: js/src/xpconnect/src/xpcjsruntime.cpp
@@ -248,5 @@
>  {
> -    if (waiverWrapperMap)
> -        delete waiverWrapperMap;
> -    if (expandoMap)
> -        delete expandoMap;

And this isn't owned by JS either - xpconnect is it's own module. Try gal, mrbkap or bz.
Attachment #530911 - Flags: review?(pbiggar) → review+
Comment on attachment 530911 [details] [diff] [review]
Part B: Changes to js/, for landing on TM

Brendan, please may you review the jsd part of this patch. Also, do jsd changes land on m-c or TM?

Boris, please may you review the xpconnect part. As above, m-c or TM?

Thanks!
Attachment #530911 - Flags: review?(bzbarsky)
Attachment #530911 - Flags: review?(brendan)
Comment on attachment 530911 [details] [diff] [review]
Part B: Changes to js/, for landing on TM

r=me on the xpconnect parts; I don't think it matters where they land.
Attachment #530911 - Flags: review?(bzbarsky) → review+
Attachment #530960 - Flags: review?(benjamin) → review+
Attachment #530914 - Flags: review?(shaver) → review?(benjamin)
Created attachment 532637 [details] [diff] [review]
Part A: For mozilla-central [checked-in]

Only changes are updating to tip and adding r=bsmedberg to commit message.
Carrying forwards r+.

Part A ready to check in.
Attachment #530960 - Attachment is obsolete: true
Attachment #532637 - Flags: review+
Keywords: checkin-needed
Whiteboard: [Please check in part A only]
Comment on attachment 530914 [details] [diff] [review]
Part C: Unnecessary null outs; for m-c

>diff --git a/browser/components/migration/src/nsOperaProfileMigrator.cpp b/browser/components/migration/src/nsOperaProfileMigrator.cpp

>-  if (ocm) {
>-    delete ocm;
>-    ocm = nsnull;
>-  }
>+  delete ocm;

That's not equivalent, and might cause a double-delete...

>diff --git a/xpcom/build/nsXPComInit.cpp b/xpcom/build/nsXPComInit.cpp

>-    if (sIOThread) {
>-        delete sIOThread;
>-        sIOThread = nsnull;
>-    }
>+    delete sIOThread;

Here also!
Attachment #530914 - Flags: review?(benjamin) → review-
@ bsmedberg

(In reply to comment #34)
> That's not equivalent, and might cause a double-delete...

The changes in the part C patch deliberately remove the =nsnull part, since they appeared unneeded - which then just leaves the null check before delete, which is removed like in the part A patch.

Or have I misunderstood comment 20 through to comment 23? (If so, sorry for the confusion, I'm still quite new to C++!)
"delete ocm" doesn't set ocm to null. The original code does.
(In reply to comment #36)
> "delete ocm" doesn't set ocm to null. The original code does.

Yeah I realise delete doesn't set ocm to null, but comment 21 implied that particular null out wasn't needed, since the variable was never used again, which is why I deliberately removed it. Or is comment 21 mistaken?
In general I prefer to null out members when they are deleted whether it is necessary or not: the null costs almost nothing and is good defensive practice.
Attachment #530914 - Attachment is obsolete: true
That makes sense, thank you for the explanation :-)

Comment 40

6 years ago
(In reply to comment #39)
> That makes sense, thank you for the explanation :-)

Except that ocm is a local variable, not a member variable. But someone could add code later in that function that tried to use the local variable, I guess.
Keywords: checkin-needed
Whiteboard: [Please check in part A only] → [fixed in cedar][only part A]
(In reply to comment #40)
> Except that ocm is a local variable, not a member variable.

And in that case defense-in-depth hurts even less, because any decent optimizing compiler will remove the null-write for you.  (Although if that's the case, nulling it out seems purely nugatory, because it won't /actually/ be nulled out.)  Not that I actually much care one way or the other, to be clear.
Part A pushed:
http://hg.mozilla.org/mozilla-central/rev/c8d5469bed72
Whiteboard: [fixed in cedar][only part A]
Attachment #532637 - Attachment description: Part A: For mozilla-central → Part A: For mozilla-central [checked-in]
Brendan, ping for review (four line change) - thanks :-)

(In reply to comment #31)
> Comment on attachment 530911 [details] [diff] [review] [review]
> Part B: Changes to js/, for landing on TM


Also:
> do jsd changes land on m-c or TM?
Sorry spam, but should have clarified the review is just for the jsd changes in the part B patch.
Attachment #530911 - Flags: review?(brendan) → review+
Comment on attachment 530911 [details] [diff] [review]
Part B: Changes to js/, for landing on TM

>diff --git a/js/src/jsweakmap.cpp b/js/src/jsweakmap.cpp
>--- a/js/src/jsweakmap.cpp
>+++ b/js/src/jsweakmap.cpp
>@@ -291,18 +291,17 @@ WeakMap::sweep(JSContext *cx)
> 
>     rt->gcWeakMapList = NULL;
> }
> 
> void
> WeakMap::finalize(JSContext *cx, JSObject *obj)
> {
>     WeakMap *table = fromJSObject(obj);
>-    if (table)
>-        delete table;
>+    delete table;
> }

This hunk is no longer correct -- see bug 655368.

/be
Thanks for the heads up - looks like that landed in the 3 weeks since review was requested. I'll take that bit out when I update to tip :-)
Created attachment 536555 [details] [diff] [review]
Part B: Changes to js/, for landing on TM [checked in]

Updated to TM tip, removed the now not applicable hunk (comment 45 and comment 46).

No other changes, carrying forwards r+.

For whomever pushes this, it needs to land on tracemonkey + it has passed try (comment 27).

NB:
Attachment #530911 - Attachment is obsolete: true
Attachment #536555 - Flags: review+
Keywords: checkin-needed
Whiteboard: [just part B, needs to land on TM]
Comment on attachment 536555 [details] [diff] [review]
Part B: Changes to js/, for landing on TM [checked in]

http://hg.mozilla.org/tracemonkey/rev/38f994208a46
Attachment #536555 - Attachment description: Part B: Changes to js/, for landing on TM → Part B: Changes to js/, for landing on TM [checked in]
Keywords: checkin-needed
Whiteboard: [just part B, needs to land on TM] → fixed-in-tracemonkey
(In reply to comment #48)
> Comment on attachment 536555 [details] [diff] [review] [review]
> Part B: Changes to js/, for landing on TM [checked in]
> 
> http://hg.mozilla.org/tracemonkey/rev/38f994208a46

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