diff options
Diffstat (limited to 'compilerplugins')
| -rw-r--r-- | compilerplugins/clang/plugin.cxx | 6 | ||||
| -rw-r--r-- | compilerplugins/clang/plugin.hxx | 1 | ||||
| -rw-r--r-- | compilerplugins/clang/sharedvisitor/dummyplugin.hxx | 7 | ||||
| -rw-r--r-- | compilerplugins/clang/simplifypointertobool.cxx | 334 | ||||
| -rw-r--r-- | compilerplugins/clang/test/simplifypointertobool.cxx | 15 |
5 files changed, 349 insertions, 14 deletions
diff --git a/compilerplugins/clang/plugin.cxx b/compilerplugins/clang/plugin.cxx index 0ffab3ba36bd..5248927e5d84 100644 --- a/compilerplugins/clang/plugin.cxx +++ b/compilerplugins/clang/plugin.cxx @@ -837,17 +837,13 @@ bool isSmartPointerType(const Expr* e) if (tc1.ClassOrStruct("unique_ptr").StdNamespace() || tc1.ClassOrStruct("shared_ptr").StdNamespace()) return true; - return isSmartPointerType(e->getType().getTypePtr()); -} -bool isSmartPointerType(const clang::Type* t) -{ // Then check the object type coerced to the type of the get member function, in // case the type-as-written is derived from one of these types (tools::SvRef is // final, but the rest are not; but note that this will fail when the type-as- // written is derived from std::unique_ptr or std::shared_ptr for which the get // member function is declared at a base class): - auto const tc2 = loplugin::TypeCheck(t); + auto const tc2 = loplugin::TypeCheck(e->getType()); if (tc2.ClassOrStruct("unique_ptr").StdNamespace() || tc2.ClassOrStruct("shared_ptr").StdNamespace() || tc2.Class("Reference").Namespace("uno").Namespace("star") diff --git a/compilerplugins/clang/plugin.hxx b/compilerplugins/clang/plugin.hxx index 2d43371feede..66d22adf97d6 100644 --- a/compilerplugins/clang/plugin.hxx +++ b/compilerplugins/clang/plugin.hxx @@ -319,7 +319,6 @@ int derivedFromCount(const CXXRecordDecl* subtypeRecord, const CXXRecordDecl* ba // if it contained the 'extern' specifier: bool hasExternalLinkage(VarDecl const * decl); -bool isSmartPointerType(const clang::Type*); bool isSmartPointerType(const Expr*); } // namespace diff --git a/compilerplugins/clang/sharedvisitor/dummyplugin.hxx b/compilerplugins/clang/sharedvisitor/dummyplugin.hxx index ce5b352384b1..b24157965df2 100644 --- a/compilerplugins/clang/sharedvisitor/dummyplugin.hxx +++ b/compilerplugins/clang/sharedvisitor/dummyplugin.hxx @@ -37,8 +37,15 @@ public: bool TraverseDecl( Decl* ) { return complain(); } bool TraverseLinkageSpecDecl( LinkageSpecDecl* ) { return complain(); } bool TraverseStmt( Stmt* ) { return complain(); } + bool TraverseIfStmt( IfStmt* ) { return complain(); } + bool TraverseWhileStmt( WhileStmt* ) { return complain(); } + bool TraverseDoStmt( DoStmt* ) { return complain(); } + bool TraverseForStmt( ForStmt* ) { return complain(); } + bool TraverseParenExpr( ParenExpr* ) { return complain(); } bool TraverseUnaryLNot( UnaryOperator* ) { return complain(); } bool TraverseBinLAnd( BinaryOperator* ) { return complain(); } + bool TraverseBinLOr( BinaryOperator* ) { return complain(); } + bool TraverseConditionalOperator( ConditionalOperator* ) { return complain(); } bool TraverseCXXCatchStmt( CXXCatchStmt* ) { return complain(); } bool TraverseCXXDestructorDecl( CXXDestructorDecl* ) { return complain(); } bool TraverseFunctionDecl( FunctionDecl* ) { return complain(); } diff --git a/compilerplugins/clang/simplifypointertobool.cxx b/compilerplugins/clang/simplifypointertobool.cxx index da6581915a1e..4fc4901c3efe 100644 --- a/compilerplugins/clang/simplifypointertobool.cxx +++ b/compilerplugins/clang/simplifypointertobool.cxx @@ -7,7 +7,10 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +#include <algorithm> #include <cassert> +#include <deque> +#include <stack> #include <string> #include <iostream> #include <fstream> @@ -16,6 +19,7 @@ #include <clang/AST/CXXInheritance.h> #include "plugin.hxx" #include "check.hxx" +#include "compat.hxx" /** Simplify boolean expressions involving smart pointers e.g. @@ -27,11 +31,11 @@ namespace { -class SimplifyPointerToBool : public loplugin::FilteringPlugin<SimplifyPointerToBool> +class SimplifyPointerToBool : public loplugin::FilteringRewritePlugin<SimplifyPointerToBool> { public: explicit SimplifyPointerToBool(loplugin::InstantiationData const& data) - : FilteringPlugin(data) + : FilteringRewritePlugin(data) { } @@ -42,6 +46,280 @@ public: } bool VisitImplicitCastExpr(ImplicitCastExpr const*); + + bool PreTraverseUnaryLNot(UnaryOperator* expr) + { + contextuallyConvertedExprs_.push_back(expr->getSubExpr()->IgnoreParenImpCasts()); + return true; + } + + bool PostTraverseUnaryLNot(UnaryOperator*, bool) + { + assert(!contextuallyConvertedExprs_.empty()); + contextuallyConvertedExprs_.pop_back(); + return true; + } + + bool TraverseUnaryLNot(UnaryOperator* expr) + { + auto res = PreTraverseUnaryLNot(expr); + assert(res); + res = FilteringRewritePlugin::TraverseUnaryLNot(expr); + PostTraverseUnaryLNot(expr, res); + return res; + } + + bool PreTraverseBinLAnd(BinaryOperator* expr) + { + contextuallyConvertedExprs_.push_back(expr->getLHS()->IgnoreParenImpCasts()); + contextuallyConvertedExprs_.push_back(expr->getRHS()->IgnoreParenImpCasts()); + return true; + } + + bool PostTraverseBinLAnd(BinaryOperator*, bool) + { + assert(contextuallyConvertedExprs_.size() >= 2); + contextuallyConvertedExprs_.pop_back(); + contextuallyConvertedExprs_.pop_back(); + return true; + } + + bool TraverseBinLAnd(BinaryOperator* expr) + { + auto res = PreTraverseBinLAnd(expr); + assert(res); + res = FilteringRewritePlugin::TraverseBinLAnd(expr); + PostTraverseBinLAnd(expr, res); + return res; + } + + bool PreTraverseBinLOr(BinaryOperator* expr) + { + contextuallyConvertedExprs_.push_back(expr->getLHS()->IgnoreParenImpCasts()); + contextuallyConvertedExprs_.push_back(expr->getRHS()->IgnoreParenImpCasts()); + return true; + } + + bool PostTraverseBinLOr(BinaryOperator*, bool) + { + assert(contextuallyConvertedExprs_.size() >= 2); + contextuallyConvertedExprs_.pop_back(); + contextuallyConvertedExprs_.pop_back(); + return true; + } + + bool TraverseBinLOr(BinaryOperator* expr) + { + auto res = PreTraverseBinLOr(expr); + assert(res); + res = FilteringRewritePlugin::TraverseBinLOr(expr); + PostTraverseBinLOr(expr, res); + return res; + } + + bool PreTraverseConditionalOperator(ConditionalOperator* expr) + { + contextuallyConvertedExprs_.push_back(expr->getCond()->IgnoreParenImpCasts()); + return true; + } + + bool PostTraverseConditionalOperator(ConditionalOperator*, bool) + { + assert(!contextuallyConvertedExprs_.empty()); + contextuallyConvertedExprs_.pop_back(); + return true; + } + + bool TraverseConditionalOperator(ConditionalOperator* expr) + { + auto res = PreTraverseConditionalOperator(expr); + assert(res); + res = FilteringRewritePlugin::TraverseConditionalOperator(expr); + PostTraverseConditionalOperator(expr, res); + return res; + } + + bool PreTraverseIfStmt(IfStmt* stmt) + { + contextuallyConvertedExprs_.push_back(stmt->getCond()->IgnoreParenImpCasts()); + return true; + } + + bool PostTraverseIfStmt(IfStmt*, bool) + { + assert(!contextuallyConvertedExprs_.empty()); + contextuallyConvertedExprs_.pop_back(); + return true; + } + + bool TraverseIfStmt(IfStmt* stmt) + { + auto res = PreTraverseIfStmt(stmt); + assert(res); + res = FilteringRewritePlugin::TraverseIfStmt(stmt); + PostTraverseIfStmt(stmt, res); + return res; + } + + bool PreTraverseWhileStmt(WhileStmt* stmt) + { + contextuallyConvertedExprs_.push_back(stmt->getCond()->IgnoreParenImpCasts()); + return true; + } + + bool PostTraverseWhileStmt(WhileStmt*, bool) + { + assert(!contextuallyConvertedExprs_.empty()); + contextuallyConvertedExprs_.pop_back(); + return true; + } + + bool TraverseWhileStmt(WhileStmt* stmt) + { + auto res = PreTraverseWhileStmt(stmt); + assert(res); + res = FilteringRewritePlugin::TraverseWhileStmt(stmt); + PostTraverseWhileStmt(stmt, res); + return res; + } + + bool PreTraverseDoStmt(DoStmt* stmt) + { + contextuallyConvertedExprs_.push_back(stmt->getCond()->IgnoreParenImpCasts()); + return true; + } + + bool PostTraverseDoStmt(DoStmt*, bool) + { + assert(!contextuallyConvertedExprs_.empty()); + contextuallyConvertedExprs_.pop_back(); + return true; + } + + bool TraverseDoStmt(DoStmt* stmt) + { + auto res = PreTraverseDoStmt(stmt); + assert(res); + res = FilteringRewritePlugin::TraverseDoStmt(stmt); + PostTraverseDoStmt(stmt, res); + return res; + } + + bool PreTraverseForStmt(ForStmt* stmt) + { + auto const e = stmt->getCond(); + if (e != nullptr) + { + contextuallyConvertedExprs_.push_back(e->IgnoreParenImpCasts()); + } + return true; + } + + bool PostTraverseForStmt(ForStmt* stmt, bool) + { + if (stmt->getCond() != nullptr) + { + assert(!contextuallyConvertedExprs_.empty()); + contextuallyConvertedExprs_.pop_back(); + } + return true; + } + + bool TraverseForStmt(ForStmt* stmt) + { + auto res = PreTraverseForStmt(stmt); + assert(res); + res = FilteringRewritePlugin::TraverseForStmt(stmt); + PostTraverseForStmt(stmt, res); + return res; + } + + bool PreTraverseParenExpr(ParenExpr* expr) + { + parenExprs_.push(expr); + return true; + } + + bool PostTraverseParenExpr(ParenExpr*, bool) + { + assert(!parenExprs_.empty()); + parenExprs_.pop(); + return true; + } + + bool TraverseParenExpr(ParenExpr* expr) + { + auto res = PreTraverseParenExpr(expr); + assert(res); + res = FilteringRewritePlugin::TraverseParenExpr(expr); + PostTraverseParenExpr(expr, res); + return res; + } + +private: + bool isContextuallyConverted(Expr const* expr) const + { + return std::find(contextuallyConvertedExprs_.begin(), contextuallyConvertedExprs_.end(), + expr) + != contextuallyConvertedExprs_.end(); + } + + // Get the source range starting at the "."or "->" (plus any preceding non-comment white space): + SourceRange getCallSourceRange(CXXMemberCallExpr const* expr) const + { + if (expr->getImplicitObjectArgument() == nullptr) + { + //TODO: Arguably, such a call of a `get` member function from within some member + // function (so that syntactically no caller is mentioned) should already be handled + // differently when reporting it (just "drop the get()" does not make sense), instead of + // being fitered here: + return {}; + } + // CXXMemberCallExpr::getExprLoc happens to return the location following the "." or "->": + auto start = compiler.getSourceManager().getSpellingLoc(expr->getExprLoc()); + if (!start.isValid()) + { + return {}; + } + for (;;) + { + start = Lexer::GetBeginningOfToken(start.getLocWithOffset(-1), + compiler.getSourceManager(), compiler.getLangOpts()); + auto const s = StringRef(compiler.getSourceManager().getCharacterData(start), + Lexer::MeasureTokenLength(start, compiler.getSourceManager(), + compiler.getLangOpts())); + if (s.empty() || s.startswith("\\\n")) + { + continue; + } + if (s != "." && s != "->") + { + return {}; + } + break; + } + for (;;) + { + auto start1 = Lexer::GetBeginningOfToken( + start.getLocWithOffset(-1), compiler.getSourceManager(), compiler.getLangOpts()); + auto const s = StringRef(compiler.getSourceManager().getCharacterData(start1), + Lexer::MeasureTokenLength(start1, compiler.getSourceManager(), + compiler.getLangOpts())); + if (!(s.empty() || s.startswith("\\\n"))) + { + break; + } + start = start1; + } + return SourceRange(start, + compiler.getSourceManager().getSpellingLoc(compat::getEndLoc(expr))); + } + + //TODO: There are some more places where an expression is contextually converted to bool, but + // those are probably not relevant for our needs here. + std::deque<Expr const*> contextuallyConvertedExprs_; + + std::stack<ParenExpr const*> parenExprs_; }; bool SimplifyPointerToBool::VisitImplicitCastExpr(ImplicitCastExpr const* castExpr) @@ -58,7 +336,7 @@ bool SimplifyPointerToBool::VisitImplicitCastExpr(ImplicitCastExpr const* castEx return true; // castExpr->dump(); // methodDecl->getParent()->getTypeForDecl()->dump(); - if (!loplugin::isSmartPointerType(methodDecl->getParent()->getTypeForDecl())) + if (!loplugin::isSmartPointerType(memberCallExpr->getImplicitObjectArgument())) return true; // if (isa<CXXOperatorCallExpr>(callExpr)) // return true; @@ -104,15 +382,57 @@ bool SimplifyPointerToBool::VisitImplicitCastExpr(ImplicitCastExpr const* castEx // if (isa<ConditionalOperator>(arg->IgnoreParenImpCasts())) // continue; // } - report(DiagnosticsEngine::Warning, "simplify, drop the get()", castExpr->getExprLoc()) - << castExpr->getSourceRange(); + if (isContextuallyConverted(memberCallExpr)) + { + if (rewriter) + { + auto const range = getCallSourceRange(memberCallExpr); + if (range.isValid() && removeText(range)) + { + return true; + } + } + report(DiagnosticsEngine::Warning, "simplify, drop the get()", memberCallExpr->getExprLoc()) + << memberCallExpr->getSourceRange(); + } + else if (!parenExprs_.empty() && parenExprs_.top()->IgnoreImpCasts() == memberCallExpr) + { + //TODO: attempt rewrite + report(DiagnosticsEngine::Warning, + "simplify, drop the get() and turn the surrounding parentheses into a functional " + "cast to bool", + memberCallExpr->getExprLoc()) + << memberCallExpr->getSourceRange(); + report(DiagnosticsEngine::Note, "surrounding parentheses here", + parenExprs_.top()->getExprLoc()) + << parenExprs_.top()->getSourceRange(); + } + else + { + if (rewriter) + { + auto const loc + = compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(memberCallExpr)); + auto const range = getCallSourceRange(memberCallExpr); + if (loc.isValid() && range.isValid() && insertText(loc, "bool(") + && replaceText(range, ")")) + { + //TODO: atomically only change both or neither + return true; + } + } + report(DiagnosticsEngine::Warning, + "simplify, drop the get() and wrap the expression in a functional cast to bool", + memberCallExpr->getExprLoc()) + << memberCallExpr->getSourceRange(); + } // report(DiagnosticsEngine::Note, "method here", param->getLocation()) // << param->getSourceRange(); return true; } -loplugin::Plugin::Registration<SimplifyPointerToBool> - simplifypointertobool("simplifypointertobool"); +loplugin::Plugin::Registration<SimplifyPointerToBool> simplifypointertobool("simplifypointertobool", + true); } // namespace diff --git a/compilerplugins/clang/test/simplifypointertobool.cxx b/compilerplugins/clang/test/simplifypointertobool.cxx index 901262c3d474..18e1da607bb8 100644 --- a/compilerplugins/clang/test/simplifypointertobool.cxx +++ b/compilerplugins/clang/test/simplifypointertobool.cxx @@ -11,11 +11,24 @@ void foo(); -void test1(std::unique_ptr<int> p2) +bool test1(std::unique_ptr<int> p2) { // expected-error@+1 {{simplify, drop the get() [loplugin:simplifypointertobool]}} if (p2.get()) foo(); + // expected-error@+1 {{simplify, drop the get() and wrap the expression in a functional cast to bool [loplugin:simplifypointertobool]}} + bool b1 = p2.get(); + //TODO: + bool b2 = ( // deliberately spread across multiple lines + p2.get()); + return b1 && b2; +} + +void test2(std::shared_ptr<int> p) +{ + // expected-error@+1 {{simplify, drop the get() [loplugin:simplifypointertobool]}} + if (p.get()) + foo(); } /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ |