Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Static analysis: Point out member shadowing

NEW
Unassigned

Status

()

Core
Rewriting and Analysis
--
enhancement
8 years ago
7 years ago

People

(Reporter: (dormant account), Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
Can we get this running on the static analysis box?
If it runs as part of a standard --with-static-checking build we can!
(Reporter)

Comment 3

8 years ago
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
Attachment #406754 - Attachment is obsolete: true
(Reporter)

Updated

8 years ago
Depends on: 525094
(Reporter)

Comment 4

8 years ago
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.
Attachment #408935 - Attachment is obsolete: true
Attachment #410019 - Flags: review?(benjamin)
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.
Attachment #410019 - Flags: review?(benjamin) → review+
(Reporter)

Comment 6

8 years ago
(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.
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.
(Reporter)

Comment 8

8 years ago
Created attachment 410043 [details] [diff] [review]
added params testcase

carrying over Benjamin's review
Attachment #410043 - Flags: review+
(Reporter)

Comment 9

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

Honza, please review the change in security/.
Attachment #410043 - Flags: review?(honzab.moz)
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.
Attachment #410043 - Flags: review?(honzab.moz) → review-
(Reporter)

Comment 11

8 years ago
(In reply to comment #10)

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

Thanks, can you make it block this bug?
You need to log in before you can comment on or make changes to this bug.