Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

delete to unqualified name woes

#153

I think in attempt to prevent memory leaks, a lot of the code involving frames and scopes does this:

delete foo;

In ECMA5, this is deprecated. You can't delete from local scope, only qualified properties (foo.bar). And jslint s unhappy about it too ("expected '.' got XXX).

JS warning #235 in env.js at line 20480 ch 8 - Applying the 'delete' operator to an unqualified name is deprecated
JS warning #235 in env.js at line 20486 ch 8 - Applying the 'delete' operator to an unqualified name is deprecated
JS warning #235 in env.js at line 20488 ch 4 - Applying the 'delete' operator to an unqualified name is deprecated
JS warning #235 in env.js at line 20552 ch 8 - Applying the 'delete' operator to an unqualified name is deprecated
JS warning #235 in env.js at line 20566 ch 12 - Applying the 'delete' operator to an unqualified name is deprecated
JS warning #235 in env.js at line 20568 ch 8 - Applying the 'delete' operator to an unqualified name is deprecated
JS warning #235 in env.js at line 20569 ch 8 - Applying the 'delete' operator to an unqualified name is deprecated
JS warning #235 in env.js at line 22740 ch 16 - Applying the 'delete' operator to an unqualified name is deprecated
JS strict warning #163 in env.js at line 22741 - useless expression

The last one is "delete this" clearly bogus.

Some of the constructs are

loop {
  tmp = something
  delete tmp;
}

I think this should be

loop {
  tmp = something
}
tmp = null;

replacing "delete foo" with "foo = null" I think will produce the desired effect.

I'm not taking this one (unless weeks go by) since I think other people are working on scoping issues, and will touch these files.

Reported by nickg · April 14th, 2010 @ 04:13 PM

State: open
Milestone: 1.2.x
Assigned to: Thatcher Thatcher

Activity

  1. Thatcher
    Thatcher
    • State changed from new to open
    • Tag set to delete
    • Assigned user set to Thatcher
    • Milestone set to 1.2.x

    checking into this, but generally I understand this issue. I'm hoping none of this is in the html 5 parser.

    April 14th, 2010 @ 05:29 PM

  2. nickg
    nickg

    howdy,

    none is in html5 parser....

    $ find src -name '*.js' | xargs grep -n 'delete '
// (edited)
src/parser/domparser.js:43:        delete tmpNode;
src/parser/domparser.js:49:        delete tmpNode;
src/parser/domparser.js:51:    delete tmpdoc,
src/parser/domparser.js:115:        delete tmpNode;
src/parser/domparser.js:129:            delete tmpNode;
src/parser/domparser.js:131:        delete tmpdoc;
src/parser/domparser.js:132:        delete htmlstring;
src/window/window.js:271:                delete scope;
src/window/window.js:272:                delete this;

    Thought there were more actually...

    April 14th, 2010 @ 05:34 PM

  3. Thatcher
    Thatcher

    I think each of these could be replaced with 'foo = null;' but I need to
    double check.

    April 14th, 2010 @ 05:37 PM

  4. gleneivey
    gleneivey
    • Tag changed from delete to garbage collection, memory leak, delete

    I just pushed 41401804df70f9e2de229a82ba081385377aa186 to thatcher/env-js/master. This eliminates all of the delete operation complaints from both jslint and "rhino -strict". Most of the deletes were obviously (a) intended as garbage-collection (even though that's not what 'delete' is for) and (b) completely non-functional--in fact, I'm amazed that some of these produced only warnings and not failures.

    I simply eliminated a lot of the delete operators, without even replacing them with "fu = null", as they were applied to local variables that shouldn't need any explicit aid for garbage collection. For the rest, I tried to figure out from context whether a "delete" or "fu = null" assignment was appropriate, and leave only one in place.

    All the existing tests run clean with these changes. I ended up not adding any new tests, but did do some interactive testing to make sure that code was doing what I thought it was before I tweaked it. Also, except for a couple of remove-outright's in window/window.js, it seemed like the delete's really weren't that closely related to scoping issues, so (obviously) I decided not to wait to make these changes until I was visiting those files later on.

    April 15th, 2010 @ 11:05 AM

  5. nickg
    nickg

    ahhh,just looked at changes

    delete foo.bar
    delete foo['bar']

    are all ok (and in one case necessary)

    it's the ones like

    delete foo
    delete this

    that are bad.

    April 16th, 2010 @ 04:56 AM

  6. nickg
    nickg

    ignore me.

    The changes seem fine. I'll look into urlparse and adjust as needed.

    thanks for doing this!

    --nickg

    April 16th, 2010 @ 05:00 AM

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.