From d4aa136e975b150add5f32013ea37aa68e9ccb57 Mon Sep 17 00:00:00 2001 From: Luboš Luňák Date: Tue, 9 Oct 2012 16:21:33 +0200 Subject: compiler plugin check for if/while/true bodies with possibly {} missing Change-Id: Ia84c70006b0b8a039b6fea27f3c5cde796f25d03 --- compilerplugins/Makefile-clang.mk | 5 +- compilerplugins/README | 12 +++ compilerplugins/clang/bodynotinblock.cxx | 147 +++++++++++++++++++++++++++++++ compilerplugins/clang/bodynotinblock.hxx | 39 ++++++++ compilerplugins/clang/compileplugin.cxx | 4 + 5 files changed, 206 insertions(+), 1 deletion(-) create mode 100644 compilerplugins/clang/bodynotinblock.cxx create mode 100644 compilerplugins/clang/bodynotinblock.hxx (limited to 'compilerplugins') diff --git a/compilerplugins/Makefile-clang.mk b/compilerplugins/Makefile-clang.mk index 9b37fb9939c7..ca95f11aaff3 100644 --- a/compilerplugins/Makefile-clang.mk +++ b/compilerplugins/Makefile-clang.mk @@ -9,7 +9,10 @@ # Make sure variables in this Makefile do not conflict with other variables (e.g. from gbuild). # The list of source files. -CLANGSRC=compileplugin.cxx unusedvariablecheck.cxx +CLANGSRC=compileplugin.cxx \ + bodynotinblock.cxx \ + unusedvariablecheck.cxx \ + # You may occassionally want to override some of these diff --git a/compilerplugins/README b/compilerplugins/README index 2430d40fb72e..50c7505dd72e 100644 --- a/compilerplugins/README +++ b/compilerplugins/README @@ -28,6 +28,18 @@ All warnings and errors are marked '[loplugin]' in the message. Additional check for unused variables. +==== Body of if/while/for not in {} ==== + +- statement aligned as second statement in if/while/for body but not in a statement block [loplugin] + +Warn about the following construct: + + if( a != 0 ) + b = 2; + c = 3; + +Here either both statements should be inside {} or the second statement in indented wrong. + == Code documentation / howtos == diff --git a/compilerplugins/clang/bodynotinblock.cxx b/compilerplugins/clang/bodynotinblock.cxx new file mode 100644 index 000000000000..9f5bf0e75c4e --- /dev/null +++ b/compilerplugins/clang/bodynotinblock.cxx @@ -0,0 +1,147 @@ +/* + * This file is part of the LibreOffice project. + * + * Based on LLVM/Clang. + * + * This file is distributed under the University of Illinois Open Source + * License. See LICENSE.TXT for details. + * + */ + +#include "bodynotinblock.hxx" + +#include + +using namespace clang; + +namespace loplugin +{ + +/* +Check for two statements that are both indented to look like a body of if/while/for +but are not inside a compound statement and thus the second one is unrelated. +*/ + +BodyNotInBlock::BodyNotInBlock( ASTContext& context ) + : context( context ) + { + } + +DiagnosticBuilder BodyNotInBlock::report( DiagnosticsEngine::Level level, StringRef message, SourceLocation loc ) + { + // Do some mappings (e.g. for -Werror) that clang does not do for custom messages for some reason. + DiagnosticsEngine& diag = context.getDiagnostics(); + if( level == DiagnosticsEngine::Warning && diag.getWarningsAsErrors()) + level = DiagnosticsEngine::Error; + if( level == DiagnosticsEngine::Error && diag.getErrorsAsFatal()) + level = DiagnosticsEngine::Fatal; + return diag.Report( loc, diag.getCustomDiagID( level, message )); + } + +void BodyNotInBlock::run() + { + TraverseDecl( context.getTranslationUnitDecl()); + } + +bool BodyNotInBlock::VisitFunctionDecl( FunctionDecl* declaration ) + { + // TODO also LO header files? or a subdir? + if( !context.getSourceManager().isFromMainFile( declaration->getLocStart())) + return true; + if( !declaration->doesThisDeclarationHaveABody()) + return true; + StmtParents parents; + traverseStatement( declaration->getBody(), parents ); + return true; + } + +void BodyNotInBlock::traverseStatement( const Stmt* stmt, StmtParents& parents ) + { + parents.push_back( stmt ); + for( ConstStmtIterator it = stmt->child_begin(); + it != stmt->child_end(); + ++it ) + { + if( *it != NULL ) // some children can be apparently NULL + { + traverseStatement( *it, parents ); // substatements first + parents.push_back( *it ); + if( const IfStmt* ifstmt = dyn_cast< IfStmt >( *it )) + { + checkBody( ifstmt->getThen(), parents, 0 ); + checkBody( ifstmt->getElse(), parents, 0 ); + } + else if( const WhileStmt* whilestmt = dyn_cast< WhileStmt >( *it )) + checkBody( whilestmt->getBody(), parents, 1 ); + else if( const ForStmt* forstmt = dyn_cast< ForStmt >( *it )) + checkBody( forstmt->getBody(), parents, 2 ); + else if( const CXXForRangeStmt* forstmt = dyn_cast< CXXForRangeStmt >( *it )) + checkBody( forstmt->getBody(), parents, 2 ); + parents.pop_back(); + } + } + assert( parents.back() == stmt ); + parents.pop_back(); + } + +void BodyNotInBlock::checkBody( const Stmt* body, const StmtParents& parents, int stmtType ) + { + if( body == NULL || parents.size() < 2 ) + return; + if( dyn_cast< CompoundStmt >( body )) + return; // if body is a compound statement, then it is in {} + // Find the next statement (in source position) after 'body'. + for( int parent_pos = parents.size() - 2; // start from grandparent + parent_pos >= 0; + --parent_pos ) + { + for( ConstStmtIterator it = parents[ parent_pos ]->child_begin(); + it != parents[ parent_pos ]->child_end(); + ) + { + if( *it == parents[ parent_pos + 1 ] ) // found grand(grand...)parent + { + // get next statement after our (grand...)parent + ++it; + while( it != parents[ parent_pos ]->child_end() && *it == NULL ) + ++it; // skip empty ones (missing 'else' bodies for example) + if( it != parents[ parent_pos ]->child_end()) + { + // TODO: If both statements come from macro expansions, they may be + // below evaluated to be in the same place (because they may be + // the result of expansion of the same macro). Analysing this including + // macro expansions would be probably more complicated, so just + // ignore that in order to avoid false positives. + if( body->getLocStart().isMacroID() && (*it)->getLocStart().isMacroID()) + return; + bool invalid1, invalid2; + unsigned bodyColumn = context.getSourceManager() + .getPresumedColumnNumber( body->getLocStart(), &invalid1 ); + unsigned nextStatementColumn = context.getSourceManager() + .getPresumedColumnNumber( (*it)->getLocStart(), &invalid2 ); + if( invalid1 || invalid2 ) + return; + if( bodyColumn == nextStatementColumn ) + { + report( DiagnosticsEngine::Warning, + "statement aligned as second statement in %select{if|while|for}0 body but not in a statement block [loplugin]", + (*it)->getLocStart()) << stmtType; + report( DiagnosticsEngine::Note, + "%select{if|while|for}0 body statement is here [loplugin]", + body->getLocStart()) << stmtType; + } + return; + } + // else we need to go higher to find the next statement + } + else + ++it; + } + // If going up would mean leaving a {} block, stop, because the } should + // make it visible the two statements are not in the same body. + if( dyn_cast< CompoundStmt >( parents[ parent_pos ] )) + return; + } + } + +} // namespace diff --git a/compilerplugins/clang/bodynotinblock.hxx b/compilerplugins/clang/bodynotinblock.hxx new file mode 100644 index 000000000000..dba82a67917e --- /dev/null +++ b/compilerplugins/clang/bodynotinblock.hxx @@ -0,0 +1,39 @@ +/* + * This file is part of the LibreOffice project. + * + * Based on LLVM/Clang. + * + * This file is distributed under the University of Illinois Open Source + * License. See LICENSE.TXT for details. + * + */ + +#ifndef BODYNOTINBLOCK_H +#define BODYNOTINBLOCK_H + +#include + +using namespace clang; + +namespace loplugin +{ + +typedef std::vector< const Stmt* > StmtParents; + +class BodyNotInBlock + : public RecursiveASTVisitor< BodyNotInBlock > + { + public: + explicit BodyNotInBlock( ASTContext& context ); + void run(); + bool VisitFunctionDecl( FunctionDecl* declaration ); + private: + void traverseStatement( const Stmt* stmt, StmtParents& parents ); + void checkBody( const Stmt* body, const StmtParents& parents, int stmtType ); + DiagnosticBuilder report( DiagnosticsEngine::Level level, StringRef message, SourceLocation loc ); + ASTContext& context; + }; + +} // namespace + +#endif // BODYNOTINBLOCK_H diff --git a/compilerplugins/clang/compileplugin.cxx b/compilerplugins/clang/compileplugin.cxx index 2b1647560117..d2e32c04a5d8 100644 --- a/compilerplugins/clang/compileplugin.cxx +++ b/compilerplugins/clang/compileplugin.cxx @@ -17,6 +17,7 @@ #include #include +#include "bodynotinblock.hxx" #include "unusedvariablecheck.hxx" using namespace clang; @@ -33,6 +34,7 @@ class PluginHandler public: explicit PluginHandler( ASTContext& context ) : rewriter( context.getSourceManager(), context.getLangOpts()) + , bodyNotInBlock( context ) , unusedVariableCheck( context ) { } @@ -40,6 +42,7 @@ class PluginHandler { if( context.getDiagnostics().hasErrorOccurred()) return; + bodyNotInBlock.run(); unusedVariableCheck.run(); // TODO also LO header files? or a subdir? if( const RewriteBuffer* buf = rewriter.getRewriteBufferFor( context.getSourceManager().getMainFileID())) @@ -48,6 +51,7 @@ class PluginHandler } private: Rewriter rewriter; + BodyNotInBlock bodyNotInBlock; UnusedVariableCheck unusedVariableCheck; }; -- cgit