Bug 354087

Comment 1 Mathias De Maré 2011-08-08 13:19:50 EDT

Created attachment 201094 [details]
Working checker (no quickfix)

- Checks for unneeded external header guards
- The patch should work fine on the master branch

The ASTPreprocessorVisitor is inspired by a 'CommenterVisitor' in the ast rewriting part.

Comment 2 Mathias De Maré 2011-08-08 13:35:16 EDT

The above is my first patch to the Eclipse project, so comments/remarks are certainly welcome.

If the patch is too large, I can split off the unit tests (~ 200 lines), but I don't think it's possible to split off any other parts.

Comment 3 Mathias De Maré 2011-08-09 11:55:50 EDT

Created attachment 201158 [details]
Headerguard checker (modified: style problem instead of programming problem)

Comment 4 Mathias De Maré 2011-08-15 14:51:04 EDT

Created attachment 201513 [details]
Headerguard patch (fix due to changes in the master branch)

Comment 5 Mathias De Maré 2011-08-15 16:39:15 EDT

I just wrote a quickfix (still needs some cleanup), but then encountered the following:

java.lang.IllegalArgumentException: Rewriting preprocessor statements is not yet supported
	at org.eclipse.cdt.core.dom.rewrite.ASTRewrite.checkSupportedNode(ASTRewrite.java:233)
	at org.eclipse.cdt.core.dom.rewrite.ASTRewrite.remove(ASTRewrite.java:124)
	at org.eclipse.cdt.codan.internal.checkers.ui.quickfix.QuickFixUnneededHeaderguard.modifyAST(QuickFixUnneededHeaderguard.java:71)

If I comment out the throwing of the exception in ASTRewrite, the checker works. Is this exception needed then? Should I remove it, change it, do something about it?

Comment 6 Mathias De Maré 2011-09-05 17:30:22 EDT

Created attachment 202773 [details]
Codan checker + quickfix

Add a codan checker for redundant headerguards, as well as a quickfix.
Unit tests are available for the checker and the quickfix.

Comment 7 Mathias De Maré 2011-09-06 14:18:19 EDT

Created attachment 202833 [details]
Codan checker + quickfix

Forgot copyright statements.

Review is certainly welcome.

Comment 8 Elena Laskavaia 2011-11-15 19:38:23 EST

I applied patch to head and tests do not pass, I also manually created a tests similar to one in tests and nothing if found.
I don't understand why you actually traverse ast nodes if only thing you need are preprocessor statements. Why just don't get list of preprocessor statements and work with them directly?

Comment 9 Elena Laskavaia 2011-11-15 20:08:46 EST

OK reason is not working for me is
java.net.URISyntaxException: Illegal character in opaque part at index 2: C:\Develop\cdt\runtime-codan\codan_c\unguard.h

I cannot open any include file. You are not using correct way of converting path to URI

I don't think this checker should actually even bother checking includes,
pattern
#ifndef INCLUDEDHEADER_H
#include "includedheader.h"
#endif

IS the pattern of external guard, it should be warning irrelevant to if include
has guard or not. Because if it does not have it should. Opening ast of all includes would be very cpu intensive, remember this checker run as you type, do you really want to do all of this
on every keystroke? If you really want you can make it an option of a problem type.

Also I am not sure that ast access is properly locked, you
require an indexer lock to access an ast for external file (it is locked for you when you are in the astchecker).

Comment 10 Mathias De Maré 2011-11-16 00:26:31 EST

Thanks for the feedback.

You have a good point about it perhaps not being necessary to go through the AST, but it can be necessary at times.

You might have something like:

#ifdef CONDITION_ONE
#include "abc.h"
#else
#include "def.h"
#endif

Then again, I guess an easier checker would still handle most of the cases.

Concerning the conversion to URI: I only tested it on Linux, so I apparently missed that.

Thanks, I will have a look and modify it to only look inside the file itself, and not traverse AST's for every include.

Comment 11 Elena Laskavaia 2011-11-16 20:12:12 EST

> 
> #ifdef CONDITION_ONE
> #include "abc.h"
> #else
> #include "def.h"
> #endif
> 
This is not a Guard pattern

The checker would miss if something like that in the include itself (i.e.
it would consider it guarded)

int a;
#ifndef X
#define X
...
#endif

But it is a problem on its own, and a rare one, so I suggest just ignore it for simplicity

Comment 12 Mathias De Maré 2011-11-20 06:54:59 EST

Created attachment 207276 [details]
Codan checker + quickfix for external header guards

I've created a new version of the checker, which only checks inside its own AST. I also created a new set of test cases to reflect this change.

Indeed, the issue you mention above is one that will be missed; but I agree that it's a rare issue that should be handled in a separate checker, if at all.

Comment 13 Elena Laskavaia 2011-11-21 20:30:49 EST

I am still not sure why your checker use ASTPreprocessorVisitor, this will work just fine


	public void processAst(IASTTranslationUnit ast) {
		ArrayList<IASTPreprocessorStatement> preprocessorStatements = new ArrayList<IASTPreprocessorStatement>(Arrays.asList(ast
				.getAllPreprocessorStatements()));
		List<CandidateIssue> candidateIssues = getCandidateIssues(preprocessorStatements);
		//Check if the candidate issues are actual issues
		for (CandidateIssue candidate : candidateIssues) {
			if (isIssue(candidate)) {
				reportProblem(ER_ID, candidate.getInclude());
			}
		}
	}

Comment 14 Mathias De Maré 2011-11-22 00:15:09 EST

Because that will add any false positive that contains code like:

#ifndef
#include
int foo();
#endif

or

#ifndef
int i;
#include
#endif

If you want, I will remove the ASTPreprocessorVisitor, but I want to add as little false positives as possible.

Comment 15 Mathias De Maré 2011-12-19 08:01:20 EST

Created attachment 208548 [details]
Smaller patch for external headerguards.

I made a smaller patch with your suggested change. All of the unit tests pass and I've done some extra manual testing on the detection and quick fix as well. It seems to work fine.

Comment 16 Sergey Prigogin 2012-04-02 20:57:45 EDT

Please make sure that the new checker is disabled by default. We can enable it later when we determine that it doesn't produce false positives.

Comment 17 Elena Laskavaia 2012-04-02 21:15:33 EDT

It does not apply cleanly to master. I manually fixed ASTRewrite but quick fix still does not work...

Comment 18 Sergey Prigogin 2012-04-02 21:23:38 EDT

ASTRewrite changes are not sufficient. ASTVisitor does not visit preprocessor statements, so they have to be handled separately in ChangeGenerator.