Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 314576

Summary: [checker] Add a simple checker for variable self-assignment
Product: [Tools] CDT Reporter: Severin Gehwolf <sgehwolf>
Component: cdt-codanAssignee: Project Inbox <cdt-core-inbox>
Status: RESOLVED FIXED QA Contact: Elena Laskavaia <elaskavaia.cdt>
Severity: normal    
Priority: P3 CC: cdt.genie, cdtdoug, overholt, sgehwolf
Version: 7.0   
Target Milestone: 8.0   
Hardware: PC   
OS: Linux   
Whiteboard:
Attachments:
Description Flags
Eclipse plugin: Assgnment to itself checker
none
Patch (project level) on org.eclipse.cdt.codan.checkers
none
Patch (workspace level) on org.eclipse.cdt.codan.{checkers,core.test} elaskavaia.cdt: iplog+

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