522776 - Static analysis: Point out member shadowing

Status: RESOLVED WORKSFORME

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[(dormant account)]

Attachment: 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!)]

Can we get this running on the static analysis box?

Comment 2

[Benjamin Smedberg]

If it runs as part of a standard --with-static-checking build we can!

Comment 3

[(dormant account)]

Attachment: 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)]

Attachment: 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]

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)]

(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]

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)]

Attachment: added params testcase

carrying over Benjamin's review

Comment 9

[(dormant account)]

Comment on attachment 410043 [details] [diff] [review]
added params testcase

Honza, please review the change in security/.

Comment 10

[Honza Bambas (:mayhemer)]

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)]

(In reply to comment #10)

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

Thanks, can you make it block this bug?

Comment 12

[Andi [:andi]]

No longer the case.