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

Bug 343429

Summary: [checker] Checker to pinpoint unused static functions in a file
Product: [Tools] CDT Reporter: Andrew Gvozdev <angvoz.dev>
Component: cdt-codanAssignee: Andrew Gvozdev <angvoz.dev>
Status: RESOLVED FIXED QA Contact: Elena Laskavaia <elaskavaia.cdt>
Severity: normal    
Priority: P3 CC: cdtdoug, malaperle
Version: 8.0Flags: malaperle: review+
Target Milestone: 8.0   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
patch
angvoz.dev: iplog-
patch 2 angvoz.dev: iplog-

Description Andrew Gvozdev CLA 2011-04-20 11:31:12 EDT 
I wish to have a checker to remove some of the cruft like unused static functions.
Comment 1 Andrew Gvozdev CLA 2011-04-22 17:22:07 EDT 
Created attachment 193949 [details]
patch

Here is the checker
Comment 2 Andrew Gvozdev CLA 2011-04-22 17:36:23 EDT 
Mark, I am not very experienced writing checkers. Could you review the code to check if I wandered astray?
Comment 3 Marc-André Laperle CLA 2011-04-24 02:13:19 EDT 
Hi Andrew.
- In collectCandidates, 'element' is cast to a IASTSimpleDeclaration and stored in 'declaration', then 'element' is cast again when adding to the lists. 'declaration' could be reused instead

- This is a false negative for 'b' : 
extern int a, b; // should be added to tests!
This leads me to think the declarators should be stored in the lists instead of declarations.

- If declarators are stored instead, maybe a HashMap<IBinding, IASTDeclarator> could be used instead of Lists to avoid having to search for the binding in a loop

- In collectCandidates, 'element' could be renamed to 'declaration' since it's a IASTDeclaration and 'declaration' could be renamed to 'simpleDeclaration' since it's a IASTSimpleDeclaration. Would be a bit easier to read.

- White spaces are a bit inconsistent. For example, storageClass == IASTDeclSpecifier.sc_extern vs name!=null. I think the default Eclipse style is with the spaces.

- I'm not sure why collectCandidates and filterOutUsedElements return Lists, they can probably be changed to return void.

- There is some filtering out in collectCandidates (in the function definition case). Could all the filtering be done in filterOutUsedElements instead? Or I am missing something?

- ... implements ICheckerWithPreferences, does it need to? I don't think so.

Is it OK to add a feature like this at this point in the release cycle? I personally can't test this checker thoroughly since the code base I work on is 99% object oriented.
Comment 4 Andrew Gvozdev CLA 2011-04-24 11:52:30 EDT 
(In reply to comment #3)
Thanks for the thorough review, I'll work to fix  the deficiencies.

> Is it OK to add a feature like this at this point in the release cycle? I
> personally can't test this checker thoroughly since the code base I work on is
> 99% object oriented.
We have not reached "Feature Freeze" milestone yet (http://www.eclipse.org/projects/project-plan.php?projectid=eclipse#release_milestones), so it should be OK to add features if there is no API change. As far as this particular feature, I believe it is quite safe to add, i.e. it is quite small and very loosely connected to the rest of the code. If there is any problem it is trivial to disable it or remove from code. I think that the biggest worry is to get flood of false positives and in that sense your code base is complimentary to ours where we got maybe ~75% of C code. I ran the checker on it and checked a few hundred of the checker warnings (out of ~6000). Found a false positive related to constructor which I fixed. I'll do another round of checking later.
Please, let me know if you still have reservations about adding to 8.0.
Comment 5 Andrew Gvozdev CLA 2011-04-26 15:30:45 EDT 
Created attachment 194095 [details]
patch 2

(In reply to comment #3)
Marc, could you review after corrections? I'd like to get this to CDT before feature freeze.

> - In collectCandidates, 'element' is cast to a IASTSimpleDeclaration and stored
> in 'declaration', then 'element' is cast again when adding to the lists.
> 'declaration' could be reused instead
Fixed

> - This is a false negative for 'b' :
> extern int a, b; // should be added to tests!
> This leads me to think the declarators should be stored in the lists instead of
> declarations.
> - If declarators are stored instead, maybe a HashMap<IBinding, IASTDeclarator>
> could be used instead of Lists to avoid having to search for the binding in a
> loop
Thanks, this also lets to simplify the code dramatically. Is there guaranteed one-to-one relationship between IBinding and IASTDeclarator?

> - In collectCandidates, 'element' could be renamed to 'declaration' since it's a
> IASTDeclaration and 'declaration' could be renamed to 'simpleDeclaration' since
> it's a IASTSimpleDeclaration. Would be a bit easier to read.
Fixed
 
> - White spaces are a bit inconsistent. For example, storageClass ==
> IASTDeclSpecifier.sc_extern vs name!=null. I think the default Eclipse style is
> with the spaces.
Fixed
 
> - I'm not sure why collectCandidates and filterOutUsedElements return Lists,
> they can probably be changed to return void.
Fixed
 
> - There is some filtering out in collectCandidates (in the function definition
> case). Could all the filtering be done in filterOutUsedElements instead? Or I am
> missing something?
If we can filter out on the first pass we should in the name of performance. In some cases the second pass won't even be triggered.

> - ... implements ICheckerWithPreferences, does it need to? I don't think so.
Fixed
Comment 6 Marc-André Laperle CLA 2011-04-28 18:36:13 EDT 
(In reply to comment #5)
> (In reply to comment #3)
> Marc, could you review after corrections? I'd like to get this to CDT before
> feature freeze. 

Sorry for the delay, I'm very busy these days. The patch looks good.

> Thanks, this also lets to simplify the code dramatically. Is there guaranteed
> one-to-one relationship between IBinding and IASTDeclarator?

In this case, I think so yes.
Comment 7 Andrew Gvozdev CLA 2011-04-28 23:06:31 EDT 
Thanks, committed on HEAD. I'll add some enhancements to cover a few peculiar false positives I discovered.
Comment 10 Andrew Gvozdev CLA 2011-04-29 15:28:48 EDT 
Committed to the HEAD more fixes to reduce false positives including unresolved types, cvs ident, extern with initializer etc. That should be it for now.