Last Comment Bug 708695 - Fix issues found by clang's scan-build
: Fix issues found by clang's scan-build
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-08 10:00 PST by Tom Schuster [:evilpie]
Modified: 2011-12-19 11:14 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (9.99 KB, patch)
2011-12-15 11:49 PST, Tom Schuster [:evilpie]
jwalden+bmo: review+
Details | Diff | Review

Description Tom Schuster [:evilpie] 2011-12-08 10:00:55 PST

    
Comment 2 Tom Schuster [:evilpie] 2011-12-15 11:49:17 PST
Created attachment 582056 [details] [diff] [review]
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] (remove +bmo to email) 2011-12-15 15:04:23 PST
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 = ...)|.

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