Community
Participate
Working Groups
Build Identifier: 8.0 I am currently working on a checker to detect unneeded external header guards. The checker is almost done. I intend to add a quick fix for this as well. External header guards (or 'external include guards) were initially suggested by John Lakos in 'Large Scale C++ Software Design'. They were meant to solve the issue of opening header files many times if they were included in a lot of other files. Compilers back then didn't optimise for this case. In more recent times, compilers detect include guards (or '#pragma once') and simply don't open the included file if it was already included. This makes the external include guards unnecessary. The only thing external include guards currently still do, is clutter up source code and make it harder to read. They also make errors possible (for example by using the wrong HEADERFILE_H in the #define). Example: includedheader.h: #ifndef INCLUDEDHEADER_H #define INCLUDEDHEADER_H int foo(); #endif header.h: #ifndef INCLUDEDHEADER_H #include "includedheader.h" #endif void bar(); Reproducible: Always
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.
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.
Created attachment 201158 [details] Headerguard checker (modified: style problem instead of programming problem)
Created attachment 201513 [details] Headerguard patch (fix due to changes in the master branch)
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?
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.
Created attachment 202833 [details] Codan checker + quickfix Forgot copyright statements. Review is certainly welcome.
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?
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).
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.
> > #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
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.
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()); } } }
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.
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.
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.
It does not apply cleanly to master. I manually fixed ASTRewrite but quick fix still does not work...
ASTRewrite changes are not sufficient. ASTVisitor does not visit preprocessor statements, so they have to be handled separately in ChangeGenerator.
