compilerplugins/clang/bodynotinblock.cxx | 81 +++++++++++-------------- compilerplugins/clang/bodynotinblock.hxx | 9 +- compilerplugins/clang/plugin.cxx | 65 ++++++++++++++++++++ compilerplugins/clang/plugin.hxx | 9 ++ compilerplugins/clang/postfixincrementfix.cxx | 83 +++++++------------------- compilerplugins/clang/postfixincrementfix.hxx | 11 +-- 6 files changed, 144 insertions(+), 114 deletions(-)
New commits: commit 597178ceecf30009c3f9098d78ab165d97b6b1f8 Author: LuboÅ¡ LuÅák <[email protected]> Date: Thu Jun 20 01:17:58 2013 +0200 simplify bodynotinblock plugin using parentStmt() Change-Id: Ia2fe10af2ca555c7b88348e7ed571f1176586580 diff --git a/compilerplugins/clang/bodynotinblock.cxx b/compilerplugins/clang/bodynotinblock.cxx index 104a146..c4780ba 100644 --- a/compilerplugins/clang/bodynotinblock.cxx +++ b/compilerplugins/clang/bodynotinblock.cxx @@ -38,50 +38,42 @@ void BodyNotInBlock::run() TraverseDecl( compiler.getASTContext().getTranslationUnitDecl()); } -bool BodyNotInBlock::VisitFunctionDecl( const FunctionDecl* declaration ) +bool BodyNotInBlock::VisitIfStmt( const IfStmt* stmt ) { - if( ignoreLocation( declaration )) + if( ignoreLocation( stmt )) return true; - if( !declaration->doesThisDeclarationHaveABody()) + checkBody( stmt->getThen(), stmt->getIfLoc(), 0, stmt->getElse() != NULL ); + checkBody( stmt->getElse(), stmt->getElseLoc(), 0 ); + return true; + } + +bool BodyNotInBlock::VisitWhileStmt( const WhileStmt* stmt ) + { + if( ignoreLocation( stmt )) return true; - StmtParents parents; - traverseStatement( declaration->getBody(), parents ); + checkBody( stmt->getBody(), stmt->getWhileLoc(), 1 ); return true; } -void BodyNotInBlock::traverseStatement( const Stmt* stmt, StmtParents& parents ) +bool BodyNotInBlock::VisitForStmt( const ForStmt* stmt ) { - 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(), ifstmt->getIfLoc(), parents, 0, ifstmt->getElse() != NULL ); - checkBody( ifstmt->getElse(), ifstmt->getElseLoc(), parents, 0 ); - } - else if( const WhileStmt* whilestmt = dyn_cast< WhileStmt >( *it )) - checkBody( whilestmt->getBody(), whilestmt->getWhileLoc(), parents, 1 ); - else if( const ForStmt* forstmt = dyn_cast< ForStmt >( *it )) - checkBody( forstmt->getBody(), forstmt->getForLoc(), parents, 2 ); - else if( const CXXForRangeStmt* forstmt = dyn_cast< CXXForRangeStmt >( *it )) - checkBody( forstmt->getBody(), forstmt->getForLoc(), parents, 2 ); - parents.pop_back(); - } - } - assert( parents.back() == stmt ); - parents.pop_back(); + if( ignoreLocation( stmt )) + return true; + checkBody( stmt->getBody(), stmt->getForLoc(), 2 ); + return true; + } + +bool BodyNotInBlock::VisitCXXForRangeStmt( const CXXForRangeStmt* stmt ) + { + if( ignoreLocation( stmt )) + return true; + checkBody( stmt->getBody(), stmt->getForLoc(), 2 ); + return true; } -void BodyNotInBlock::checkBody( const Stmt* body, SourceLocation stmtLocation, const StmtParents& parents, - int stmtType, bool dontGoUp ) +void BodyNotInBlock::checkBody( const Stmt* body, SourceLocation stmtLocation, int stmtType, bool dontGoUp ) { - if( body == NULL || parents.size() < 2 ) + if( body == NULL ) return; // TODO: If the if/else/while/for comes from a macro expansion, ignore it completely for // now. The code below could assume everything is in the same place (and thus also column) @@ -92,22 +84,24 @@ void BodyNotInBlock::checkBody( const Stmt* body, SourceLocation stmtLocation, c return; if( dyn_cast< CompoundStmt >( body )) return; // if body is a compound statement, then it is in {} + const Stmt* previousParent = parentStmt( body ); // Here the statement itself. // 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(;;) { - for( ConstStmtIterator it = parents[ parent_pos ]->child_begin(); - it != parents[ parent_pos ]->child_end(); + const Stmt* parent = parentStmt( previousParent ); + if( parent == NULL ) + break; + for( ConstStmtIterator it = parent->child_begin(); + it != parent->child_end(); ) { - if( *it == parents[ parent_pos + 1 ] ) // found grand(grand...)parent + if( *it == previousParent ) // found grand(grand...)parent { // get next statement after our (grand...)parent ++it; - while( it != parents[ parent_pos ]->child_end() && *it == NULL ) + while( it != parent->child_end() && *it == NULL ) ++it; // skip empty ones (missing 'else' bodies for example) - if( it != parents[ parent_pos ]->child_end()) + if( it != parent->child_end()) { bool invalid1, invalid2; unsigned bodyColumn = compiler.getSourceManager() @@ -134,13 +128,14 @@ void BodyNotInBlock::checkBody( const Stmt* body, SourceLocation stmtLocation, c } // 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 ] )) + if( dyn_cast< CompoundStmt >( parent )) return; // If the body to be checked is a body of an if statement that has also // an else part, don't go up, the else is after the body and should make // it clear the body does not continue there. if( dontGoUp ) return; + previousParent = parent; } } diff --git a/compilerplugins/clang/bodynotinblock.hxx b/compilerplugins/clang/bodynotinblock.hxx index e6ad1ab..41eca7d 100644 --- a/compilerplugins/clang/bodynotinblock.hxx +++ b/compilerplugins/clang/bodynotinblock.hxx @@ -23,12 +23,13 @@ class BodyNotInBlock public: explicit BodyNotInBlock( CompilerInstance& compiler ); virtual void run() override; - bool VisitFunctionDecl( const FunctionDecl* declaration ); + bool VisitIfStmt( const IfStmt* stmt ); + bool VisitWhileStmt( const WhileStmt* stmt ); + bool VisitForStmt( const ForStmt* stmt ); + bool VisitCXXForRangeStmt( const CXXForRangeStmt* stmt ); private: typedef vector< const Stmt* > StmtParents; - void traverseStatement( const Stmt* stmt, StmtParents& parents ); - void checkBody( const Stmt* body, SourceLocation stmtLocation, const StmtParents& parents, - int stmtType, bool dontGoUp = false ); + void checkBody( const Stmt* body, SourceLocation stmtLocation, int stmtType, bool dontGoUp = false ); }; } // namespace commit 81b58bb075313ce5cb7268fa3427d977e4b2692c Author: LuboÅ¡ LuÅák <[email protected]> Date: Thu Jun 20 00:31:37 2013 +0200 simplify postfixincrementfix plugin using parentStmt() Change-Id: I93fa422afe7f3e1e10576dd64af9d57b2302f44e diff --git a/compilerplugins/clang/postfixincrementfix.cxx b/compilerplugins/clang/postfixincrementfix.cxx index edb31e3..ca636b9 100644 --- a/compilerplugins/clang/postfixincrementfix.cxx +++ b/compilerplugins/clang/postfixincrementfix.cxx @@ -29,40 +29,20 @@ void PostfixIncrementFix::run() TraverseDecl( compiler.getASTContext().getTranslationUnitDecl()); } -bool PostfixIncrementFix::VisitFunctionDecl( const FunctionDecl* declaration ) +bool PostfixIncrementFix::VisitCXXOperatorCallExpr( const CXXOperatorCallExpr* op ) { - if( ignoreLocation( declaration )) + if( ignoreLocation( op )) return true; - if( !declaration->doesThisDeclarationHaveABody()) - return true; - StmtParents parents; - fixPostfixOperators( declaration->getBody(), parents ); - return true; - } - -void PostfixIncrementFix::fixPostfixOperators( const Stmt* stmt, StmtParents& parents ) - { - if( const CXXOperatorCallExpr* op = dyn_cast<CXXOperatorCallExpr>( stmt )) - { // postfix ++ has two arguments (the operand and the hidden extra int) - if( op->getOperator() == OO_PlusPlus && op->getNumArgs() == 2 ) - fixPostfixOperator( op, parents ); - } + // postfix ++ has two arguments (the operand and the hidden extra int) + if( op->getOperator() == OO_PlusPlus && op->getNumArgs() == 2 ) + fixPostfixOperator( op ); // For primitive types it would be UnaryOperatorExpr, but probably no good reason to change those. - parents.push_back( stmt ); - for( ConstStmtIterator it = stmt->child_begin(); - it != stmt->child_end(); - ++it ) - { - if( *it != NULL ) // some children can be apparently NULL - fixPostfixOperators( *it, parents ); - } - assert( parents.back() == stmt ); - parents.pop_back(); + return true; } -void PostfixIncrementFix::fixPostfixOperator( const CXXOperatorCallExpr* op, StmtParents& parents ) +void PostfixIncrementFix::fixPostfixOperator( const CXXOperatorCallExpr* op ) { - if( !canChangePostfixToPrefix( op, parents, parents.size() - 1 )) + if( !canChangePostfixToPrefix( op, op )) return; if( !shouldDoChange( op->getArg( 0 ))) return; @@ -73,10 +53,13 @@ void PostfixIncrementFix::fixPostfixOperator( const CXXOperatorCallExpr* op, Stm removeText( op->getCallee()->getSourceRange()); } -bool PostfixIncrementFix::canChangePostfixToPrefix( const CXXOperatorCallExpr* op, StmtParents& parents, int parent_pos ) +bool PostfixIncrementFix::canChangePostfixToPrefix( const Stmt* stmt , const CXXOperatorCallExpr* op ) { + const Stmt* parent = parentStmt( stmt ); + if( parent == NULL ) + return true; // check if foo++ can be safely replaced by ++foo - switch( parents[ parent_pos ]->getStmtClass()) + switch( parent->getStmtClass()) { case Stmt::CompoundStmtClass: return true; @@ -91,52 +74,32 @@ bool PostfixIncrementFix::canChangePostfixToPrefix( const CXXOperatorCallExpr* o case Stmt::CXXBindTemporaryExprClass: // tricky, it may just mean the temporary will be cleaned up // (by ExprWithCleanups), ignore and go up - assert( parent_pos > 0 ); // should not happen - return canChangePostfixToPrefix( op, parents, parent_pos - 1 ); + return canChangePostfixToPrefix( parent, op ); case Stmt::ExprWithCleanupsClass: // cleanup of a temporary, should be harmless (if the use // of the postfix ++ operator here relies on the fact that // the dtor for the object will be called, that's pretty insane // code). Ignore and go up. - assert( parent_pos > 0 ); // should not happen - return canChangePostfixToPrefix( op, parents, parent_pos - 1 ); + return canChangePostfixToPrefix( parent, op ); case Stmt::ParenExprClass: // parentheses, go up - assert( parent_pos > 0 ); - return canChangePostfixToPrefix( op, parents, parent_pos - 1 ); + return canChangePostfixToPrefix( parent, op ); case Stmt::IfStmtClass: - return canChangeInConditionStatement( op, cast< IfStmt >( parents[ parent_pos ] )->getCond(), - parents, parent_pos ); + // cannot be changed in condition, can be changed in statements + return cast< IfStmt >( parent )->getCond() != stmt; case Stmt::WhileStmtClass: - return canChangeInConditionStatement( op, cast< WhileStmt >( parents[ parent_pos ] )->getCond(), - parents, parent_pos ); + return cast< WhileStmt >( parent )->getCond() != stmt; case Stmt::DoStmtClass: - return canChangeInConditionStatement( op, cast< DoStmt >( parents[ parent_pos ] )->getCond(), - parents, parent_pos ); + return cast< DoStmt >( parent )->getCond() != stmt; case Stmt::ForStmtClass: - return canChangeInConditionStatement( op, dyn_cast< ForStmt >( parents[ parent_pos ] )->getCond(), - parents, parent_pos ); + return cast< ForStmt >( parent )->getCond() != stmt; default: report( DiagnosticsEngine::Fatal, "cannot analyze operator++ (plugin needs fixing)", - op->getLocStart()) << parents[ parent_pos ]->getSourceRange(); -// parents[ parent_pos ]->dump(); -// parents[ std::max( parent_pos - 3, 0 ) ]->dump(); + op->getLocStart()) << parent->getSourceRange(); + parent->dump(); return false; } } -bool PostfixIncrementFix::canChangeInConditionStatement( const CXXOperatorCallExpr* op, const Expr* condition, - const StmtParents& parents, unsigned int parent_pos ) - { - // cannot be changed in condition, can be changed in statements - if( parent_pos == parents.size() - 1 ) - return op != condition; - else - { // indirect child - assert( parent_pos + 1 < parents.size()); - return parents[ parent_pos + 1 ] != condition; - } - } - bool PostfixIncrementFix::shouldDoChange( const Expr* operand ) { // TODO Changing 'a->b++' to '++a->b' is technically the same, but the latter probably looks confusing, diff --git a/compilerplugins/clang/postfixincrementfix.hxx b/compilerplugins/clang/postfixincrementfix.hxx index 29756cf..e357f99 100644 --- a/compilerplugins/clang/postfixincrementfix.hxx +++ b/compilerplugins/clang/postfixincrementfix.hxx @@ -23,14 +23,11 @@ class PostfixIncrementFix public: explicit PostfixIncrementFix( CompilerInstance& compiler, Rewriter& rewriter ); virtual void run() override; - bool VisitFunctionDecl( const FunctionDecl* declaration ); + bool VisitCXXOperatorCallExpr( const CXXOperatorCallExpr* op ); private: - typedef std::vector< const Stmt* > StmtParents; - void fixPostfixOperator( const CXXOperatorCallExpr* op, StmtParents& parents ); - void fixPostfixOperators( const Stmt* stmt, StmtParents& parents ); - bool canChangePostfixToPrefix( const CXXOperatorCallExpr* op, StmtParents& parents, int parent_pos ); - bool canChangeInConditionStatement( const CXXOperatorCallExpr* op, const Expr* condition, - const StmtParents& parents, unsigned int parent_pos ); + void fixPostfixOperator( const CXXOperatorCallExpr* op ); + void fixPostfixOperators( const Stmt* stmt ); + bool canChangePostfixToPrefix( const Stmt* stmt, const CXXOperatorCallExpr* op ); bool shouldDoChange( const Expr* op ); }; commit ade47d3d67635baf9580da797370fd0e3d395b5a Author: LuboÅ¡ LuÅák <[email protected]> Date: Wed Jun 19 23:55:47 2013 +0200 make it easy to get a parent of an AST node Clang API doesn't provide this, but it's occasionally needed, and so far the way has been inspecting the highest possible node in AST and walking down and remembering, which is complicated, error-prone and annoying. Change-Id: Id5b72cb5ebfc069e90efe6d673c0ef18ebcdab61 diff --git a/compilerplugins/clang/plugin.cxx b/compilerplugins/clang/plugin.cxx index 3300908..6611606 100644 --- a/compilerplugins/clang/plugin.cxx +++ b/compilerplugins/clang/plugin.cxx @@ -67,6 +67,71 @@ void Plugin::registerPlugin( Plugin* (*create)( CompilerInstance&, Rewriter& ), PluginHandler::registerPlugin( create, optionName, isRewriter ); } +unordered_map< const Stmt*, const Stmt* > Plugin::parents; + +const Stmt* Plugin::parentStmt( const Stmt* stmt ) + { + if( parents.empty()) + buildParents( compiler ); + assert( parents.count( stmt ) == 1 ); + return parents[ stmt ]; + } + +Stmt* Plugin::parentStmt( Stmt* stmt ) + { + if( parents.empty()) + buildParents( compiler ); + assert( parents.count( stmt ) == 1 ); + return const_cast< Stmt* >( parents[ stmt ] ); + } + +namespace +{ +class ParentBuilder + : public RecursiveASTVisitor< ParentBuilder > + { + public: + bool VisitFunctionDecl( const FunctionDecl* function ); + void walk( const Stmt* stmt ); + unordered_map< const Stmt*, const Stmt* >* parents; + }; + +bool ParentBuilder::VisitFunctionDecl( const FunctionDecl* function ) + { +// if( ignoreLocation( declaration )) +// return true; ??? + if( !function->doesThisDeclarationHaveABody()) + return true; + const Stmt* body = function->getBody(); + (*parents)[ body ] = NULL; // no parent + walk( body ); + return true; + } + +void ParentBuilder::walk( const Stmt* stmt ) + { + for( ConstStmtIterator it = stmt->child_begin(); + it != stmt->child_end(); + ++it ) + { + if( *it != NULL ) + { + (*parents)[ *it ] = stmt; + walk( *it ); + } + } + } + +} // namespace + +void Plugin::buildParents( CompilerInstance& compiler ) + { + assert( parents.empty()); + ParentBuilder builder; + builder.parents = &parents; + builder.TraverseDecl( compiler.getASTContext().getTranslationUnitDecl()); + } + ///// RewritePlugin::RewritePlugin( CompilerInstance& compiler, Rewriter& rewriter ) diff --git a/compilerplugins/clang/plugin.hxx b/compilerplugins/clang/plugin.hxx index 9c9ce7b..56dee27 100644 --- a/compilerplugins/clang/plugin.hxx +++ b/compilerplugins/clang/plugin.hxx @@ -19,6 +19,7 @@ #include <clang/Basic/SourceManager.h> #include <clang/Frontend/CompilerInstance.h> #include <set> +#include <unordered_map> #if __clang_major__ < 3 || __clang_major__ == 3 && __clang_minor__ < 2 #include <clang/Rewrite/Rewriter.h> @@ -54,10 +55,18 @@ class Plugin bool ignoreLocation( const Decl* decl ); bool ignoreLocation( const Stmt* stmt ); CompilerInstance& compiler; + /** + Returns the parent of the given AST node. Clang's internal AST representation doesn't provide this information, + it can only provide children, but getting the parent is often useful for inspecting a part of the AST. + */ + const Stmt* parentStmt( const Stmt* stmt ); + Stmt* parentStmt( Stmt* stmt ); private: static void registerPlugin( Plugin* (*create)( CompilerInstance&, Rewriter& ), const char* optionName, bool isRewriter ); template< typename T > static Plugin* createHelper( CompilerInstance& compiler, Rewriter& rewriter ); enum { isRewriter = false }; + static unordered_map< const Stmt*, const Stmt* > parents; + static void buildParents( CompilerInstance& compiler ); }; /**
_______________________________________________ Libreoffice-commits mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
