Last Comment Bug 522776 - Static analysis: Point out member shadowing
: Static analysis: Point out member shadowing
Status: NEW
:
Product: Core
Classification: Components
Component: Rewriting and Analysis (show other bugs)
: Trunk
: x86 Linux
: -- enhancement with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 525094
Blocks: analyses
  Show dependency treegraph
 
Reported: 2009-10-16 13:02 PDT by (dormant account)
Modified: 2010-06-29 17:16 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
analysis (545 bytes, patch)
2009-10-16 13:02 PDT, (dormant account)
no flags Details | Diff | Review
wip (9.31 KB, patch)
2009-10-28 14:07 PDT, (dormant account)
no flags Details | Diff | Review
finished patch (11.81 KB, patch)
2009-11-03 13:10 PST, (dormant account)
benjamin: review+
Details | Diff | Review
added params testcase (12.39 KB, patch)
2009-11-03 14:24 PST, (dormant account)
taras.mozilla: review+
honzab.moz: review-
Details | Diff | Review

Description (dormant account) 2009-10-16 13:02:16 PDT
Created attachment 406754 [details] [diff] [review]
analysis

Solaris compiler warns if a stack variable shadows a class member. I got burned by this.

Filing this before i forget about it.
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-10-16 14:03:20 PDT
Can we get this running on the static analysis box?
Comment 2 Benjamin Smedberg [:bsmedberg] 2009-10-16 14:04:36 PDT
If it runs as part of a standard --with-static-checking build we can!
Comment 3 (dormant account) 2009-10-28 14:07:48 PDT
Created attachment 408935 [details] [diff] [review]
wip

make this an error is a lot of work. It trips up a lot of code. Could should probably be fixed as it doesn't follow naming conventions
Comment 4 (dormant account) 2009-11-03 13:10:51 PST
Created attachment 410019 [details] [diff] [review]
finished patch

With this patch the build can proceed. I'll probably need someone else for additional reviews on that 2 cases of member shadowing found in gtk and ssl code.
Comment 5 Benjamin Smedberg [:bsmedberg] 2009-11-03 13:15:07 PST
Comment on attachment 410019 [details] [diff] [review]
finished patch

I can't tell from this: does this error on function parameters which are named the same as members? There should probably be a test for that case, one way or the other.
Comment 6 (dormant account) 2009-11-03 13:16:38 PST
(In reply to comment #5)
> (From update of attachment 410019 [details] [diff] [review])
> I can't tell from this: does this error on function parameters which are named
> the same as members? There should probably be a test for that case, one way or
> the other.

no it doesn't. I couldn't decide if saying anything in that case would be useful or counterproductive.
Comment 7 Benjamin Smedberg [:bsmedberg] 2009-11-03 13:26:40 PST
I tend to think it would be useful but I don't care much: I just want a test so the behavior is clear and doesn't change accidentally.
Comment 8 (dormant account) 2009-11-03 14:24:35 PST
Created attachment 410043 [details] [diff] [review]
added params testcase

carrying over Benjamin's review
Comment 9 (dormant account) 2009-11-03 14:32:21 PST
Comment on attachment 410043 [details] [diff] [review]
added params testcase

Honza, please review the change in security/.
Comment 10 Honza Bambas (:mayhemer) 2009-11-04 12:01:22 PST
Comment on attachment 410043 [details] [diff] [review]
added params testcase

Taras, please rename the local variable to something else (something like nssComponentLocal, up to you). This code is old and I'm afraid it could introduce problems during shutdown/profile change, let's leave the current behavior. 

This local variable shadowing the (wrongly named!) member was introduced here: https://bugzilla.mozilla.org/attachment.cgi?id=106168&action=diff#mozilla/security/manager/ssl/src/nsUsageArrayHelper.cpp_sec1

I'll file a new bug to clean this up.
Comment 11 (dormant account) 2009-11-04 12:05:39 PST
(In reply to comment #10)

> I'll file a new bug to clean this up.

Thanks, can you make it block this bug?

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