708695 - Fix issues found by clang's scan-build

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

http://javascript-reverse.tumblr.com/post/13917377820/how-to-use-scan-build-to-analyze-spidermonkey

The actual report: http://dl.dropbox.com/u/11402872/scan-build-2011-12-08-1/index.html

Comment 2

[Tom S [:evilpie]]

Attachment: v1

This fixes most of the actual issues, except:

>Dead store	Dead initialization	methodjit /InvokeHelpers.cpp	116
bhackett or luke are going to change the function or even remove it?
>Dead store	Dead assignment	jsopcode.cpp	2200
Oh well, never mind
>Dead code	Idempotent operation	frontend /BytecodeEmitter.cpp	3184	
This just works

All the 'Dereference of null pointer' look fishy to me, but i wouldn't mind if somebody would double check.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 582056 [details] [diff] [review]
v1

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

I didn't look much at the other warnings, but some definitely look fixable with some this-function-doesn't-return annotations (assuming what I saw that looked like no-return functions actually were).  We should add MOZ_NO_RETURN to mfbt at some point for this.

::: js/src/jsinfer.cpp
@@ +4178,5 @@
>           * Don't track for parents which add call objects or are generators,
>           * don't resolve NAME accesses into the parent.
>           */
> +        if (!detached && (nesting->parent->analysis()->addsScopeObjects() ||
> +            JSOp(*nesting->parent->code) == JSOP_GENERATOR)) 

The previous wrapping was more readable.  This one suggests three equal-level terms evaluated sequentially.  That said, I don't think the previous wrapping was all that great, either.  How about this:

if (!detached) {
    /*
     * Don't track for...
     */
    if (nesting->parent->analysis()->addsScopeObjects() || JSOp(*nesting->parent()->code) == JSOP_GENERATOR)
        DetachNestingParent(script);
}

::: js/src/jsinterp.cpp
@@ +2099,1 @@
>              len = JSOP_CALL_LENGTH;

I'd somewhat prefer if this were moved next to the DO_NEXT_OP(len) below -- mind moving it?

::: js/src/jsxml.cpp
@@ +3050,2 @@
>      JS_ASSERT(list->xml_class == JSXML_CLASS_LIST);
> +    uint32 i = list->xml_kids.length;

uint32_t

@@ +3053,4 @@
>      if (xml->xml_class == JSXML_CLASS_LIST) {
>          list->xml_target = xml->xml_target;
>          list->xml_targetprop = xml->xml_targetprop;
> +        uint32 n = JSXML_LENGTH(xml);

uint32_t

@@ +3061,1 @@
>              if (kid)

Make this |if (JSXML *kid = ...)|.

Comment 4

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/caeef8ca5d94

Comment 5

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/caeef8ca5d94https://hg.mozilla.org/mozilla-central/rev/caeef8ca5d94