Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 314576 - [checker] Add a simple checker for variable self-assignment
Summary: [checker] Add a simple checker for variable self-assignment
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-codan (show other bugs)
Version: 7.0   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 8.0   Edit
Assignee: Project Inbox CLA
QA Contact: Elena Laskavaia CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-26 17:35 EDT by Severin Gehwolf CLA
Modified: 2010-07-29 08:23 EDT (History)
4 users (show)

See Also:

Attachments
Eclipse plugin: Assgnment to itself checker (6.77 KB, application/zip)
2010-05-26 17:35 EDT, Severin Gehwolf CLA 		no flags Details
Patch (project level) on org.eclipse.cdt.codan.checkers (4.22 KB, patch)
2010-05-27 11:32 EDT, Severin Gehwolf CLA 		no flags Details | Diff
Patch (workspace level) on org.eclipse.cdt.codan.{checkers,core.test} (5.60 KB, patch)
2010-05-27 15:33 EDT, Severin Gehwolf CLA 		elaskavaia.cdt: iplog+
Details | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Severin Gehwolf CLA 2010-05-26 17:35:15 EDT 
Created attachment 170107 [details]
Eclipse plugin: Assgnment to itself checker

Hi,

I've had a look at http://wiki.eclipse.org/CDT/designs/StaticAnalysis/CheckerIdeas and thought the self assignment checker would be simple enough for a first start of cdt-codan checker development. It would be really cool if somebody could have a look at it. The Eclipse plugin is attached. I wonder if there's a better way to check if variable names are the same (I am using getRawSignature() on the operator tokens at the moment).

Hopefully this will be helpful.

Cheers,
Severin
Comment 1 Elena Laskavaia CLA 2010-05-26 20:57:16 EDT 
Can you submit a patch without creating a new plugin?
Comment 2 Severin Gehwolf CLA 2010-05-27 11:32:11 EDT 
Created attachment 170205 [details]
Patch (project level) on org.eclipse.cdt.codan.checkers
Comment 3 Severin Gehwolf CLA 2010-05-27 11:33:14 EDT 
Certainly. Patch is attached. Please let me know if I did something inconvenient/unusual. Thanks!

Severin
Comment 4 CDT Genie CLA 2010-05-27 12:15:03 EDT 
Patch comments:
1) Checking rawSignature of expression is not entirely correct, i.e.
for cases like *x++ = *x++; but I could not think of example where it can be reasonably justified (i.e it is still an error).

2)
if (e instanceof IASTBinaryExpression && isAssignmentExpression(e)) {

instanceof check is redundant because isAssignmentExpression does it anyway.
Do we even need this extra function anyway (inline it?)

3)
Also description has text like
Finds expressions such as 'int value = 1; value = value;'
Should be 

Finds expression where left and right side of the assignment is the same, i.e. 'value = value'
Comment 5 Severin Gehwolf CLA 2010-05-27 12:50:17 EDT 
Thanks! The use of getRawSignature() bothered me from the start, but didn't know a better (the proper way) to do it. Any hints?
Comment 6 CDT Genie CLA 2010-05-27 14:04:58 EDT 
I think it is ok for now. We can check for only "x = x" but it would
miss interesting cases like arr[i]=arr[i]
Comment 7 Severin Gehwolf CLA 2010-05-27 15:30:36 EDT 
Actually it catches cases like a[i] = a[i].
Comment 8 Severin Gehwolf CLA 2010-05-27 15:33:16 EDT 
Created attachment 170252 [details]
Patch (workspace level) on org.eclipse.cdt.codan.{checkers,core.test}

Thanks for your feedback. I've modified the patch and incorporated your suggestions. Also, I've added a few simple tests for this checker.
Comment 9 CDT Genie CLA 2010-05-27 15:57:00 EDT 
(In reply to comment #7)
> Actually it catches cases like a[i] = a[i].

Yes this is my point, it does now because you are using getRawSignature.
And lets keep it this way.
Comment 10 Elena Laskavaia CLA 2010-05-30 19:56:06 EDT 
-> 8.0
Comment 11 Elena Laskavaia CLA 2010-07-29 08:02:18 EDT 
Patch applied on head, thanks.
Next time please add copyright header for new files and externalize strings