diff options
author | Noel Grandin <noel.grandin@collabora.co.uk> | 2018-11-16 14:01:48 +0200 |
---|---|---|
committer | Noel Grandin <noel.grandin@collabora.co.uk> | 2018-11-16 14:20:04 +0100 |
commit | faa63cd14c1d360d0e4eedb64cd879aa1229fa60 (patch) | |
tree | 332e805c8247de72145f7633c35c75c2ce0a6cc2 /compilerplugins | |
parent | f5f5a17be7bdcd0adb3928631bdeac275a5abdd9 (diff) |
new loplugin buriedassign
Change-Id: If6dd8033daf2103a81c3a7c3a44cf1e38d0a3744
Reviewed-on: https://gerrit.libreoffice.org/63466
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Diffstat (limited to 'compilerplugins')
-rw-r--r-- | compilerplugins/clang/buriedassign.cxx | 226 | ||||
-rw-r--r-- | compilerplugins/clang/test/buriedassign.cxx | 80 |
2 files changed, 306 insertions, 0 deletions
diff --git a/compilerplugins/clang/buriedassign.cxx b/compilerplugins/clang/buriedassign.cxx new file mode 100644 index 000000000000..8d0e6cc79e59 --- /dev/null +++ b/compilerplugins/clang/buriedassign.cxx @@ -0,0 +1,226 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* + * This file is part of the LibreOffice project. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +#include <cassert> +#include <string> +#include <iostream> +#include <unordered_map> +#include <unordered_set> + +#include "plugin.hxx" +#include "check.hxx" +#include "clang/AST/CXXInheritance.h" +#include "clang/AST/StmtVisitor.h" + +/** + TODO deal with C++ operator overload assign +*/ + +namespace +{ +//static bool startswith(const std::string& rStr, const char* pSubStr) { +// return rStr.compare(0, strlen(pSubStr), pSubStr) == 0; +//} +class BuriedAssign : public loplugin::FilteringPlugin<BuriedAssign> +{ +public: + explicit BuriedAssign(loplugin::InstantiationData const& data) + : FilteringPlugin(data) + { + } + + virtual void run() override + { + std::string fn(handler.getMainFileName()); + loplugin::normalizeDotDotInFilePath(fn); + // getParentStmt appears not to be working very well here + if (fn == SRCDIR "/stoc/source/inspect/introspection.cxx" + || fn == SRCDIR "/stoc/source/corereflection/criface.cxx") + return; + // calling an acquire via function pointer + if (fn == SRCDIR "/cppu/source/uno/lbenv.cxx" + || fn == SRCDIR "cppu/source/typelib/static_types.cxx") + return; + // false+, not sure why + if (fn == SRCDIR "/vcl/source/window/menu.cxx") + return; + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + } + + bool VisitBinaryOperator(BinaryOperator const*); + bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr const*); + +private: + void checkExpr(Expr const*); +}; + +static bool isAssignmentOp(clang::BinaryOperatorKind op) +{ + // We ignore BO_ShrAssign i.e. >>= because we use that everywhere for + // extracting data from css::uno::Any + return op == BO_Assign || op == BO_MulAssign || op == BO_DivAssign || op == BO_RemAssign + || op == BO_AddAssign || op == BO_SubAssign || op == BO_ShlAssign || op == BO_AndAssign + || op == BO_XorAssign || op == BO_OrAssign; +} + +static bool isAssignmentOp(clang::OverloadedOperatorKind Opc) +{ + // Same logic as CXXOperatorCallExpr::isAssignmentOp(), which our supported clang + // doesn't have yet. + // Except that we ignore OO_GreaterGreaterEqual i.e. >>= because we use that everywhere for + // extracting data from css::uno::Any + return Opc == OO_Equal || Opc == OO_StarEqual || Opc == OO_SlashEqual || Opc == OO_PercentEqual + || Opc == OO_PlusEqual || Opc == OO_MinusEqual || Opc == OO_LessLessEqual + || Opc == OO_AmpEqual || Opc == OO_CaretEqual || Opc == OO_PipeEqual; +} + +bool BuriedAssign::VisitBinaryOperator(BinaryOperator const* binaryOp) +{ + if (ignoreLocation(binaryOp)) + return true; + + if (!isAssignmentOp(binaryOp->getOpcode())) + return true; + + checkExpr(binaryOp); + return true; +} + +bool BuriedAssign::VisitCXXOperatorCallExpr(CXXOperatorCallExpr const* cxxOper) +{ + if (ignoreLocation(cxxOper)) + return true; + if (!isAssignmentOp(cxxOper->getOperator())) + return true; + checkExpr(cxxOper); + return true; +} + +void BuriedAssign::checkExpr(Expr const* binaryOp) +{ + if (compiler.getSourceManager().isMacroBodyExpansion(compat::getBeginLoc(binaryOp))) + return; + if (compiler.getSourceManager().isMacroArgExpansion(compat::getBeginLoc(binaryOp))) + return; + + /** + Note: I tried writing this plugin without getParentStmt, but in order to make that work, I had to + hack things like TraverseWhileStmt to call TraverseStmt on the child nodes myself, so I could know whether I was inside the body or the condition. + But I could not get that to work, so switched to this approach. + */ + + // look up past the temporary nodes + Stmt const* child = binaryOp; + Stmt const* parent = getParentStmt(binaryOp); + while (true) + { + // This part is not ideal, but getParentStmt() appears to fail us in some cases, notably when the assignment + // is inside a decl like: + // int x = a = 1; + if (!parent) + return; + if (!(isa<MaterializeTemporaryExpr>(parent) || isa<CXXBindTemporaryExpr>(parent) + || isa<ImplicitCastExpr>(parent) || isa<CXXConstructExpr>(parent) + || isa<ParenExpr>(parent) || isa<ExprWithCleanups>(parent))) + break; + child = parent; + parent = getParentStmt(parent); + } + + if (isa<CompoundStmt>(parent)) + return; + // ignore chained assignment like "a = b = 1;" + if (auto cxxOper = dyn_cast<CXXOperatorCallExpr>(parent)) + { + if (cxxOper->getOperator() == OO_Equal) + return; + } + // ignore chained assignment like "a = b = 1;" + if (auto parentBinOp = dyn_cast<BinaryOperator>(parent)) + { + if (parentBinOp->getOpcode() == BO_Assign) + return; + } + // ignore chained assignment like "int a = b = 1;" + if (isa<DeclStmt>(parent)) + return; + + if (isa<CaseStmt>(parent) || isa<DefaultStmt>(parent) || isa<LabelStmt>(parent) + || isa<ForStmt>(parent) || isa<CXXForRangeStmt>(parent) || isa<IfStmt>(parent) + || isa<DoStmt>(parent) || isa<WhileStmt>(parent) || isa<ReturnStmt>(parent)) + return; + + // now check for the statements where we don't care at all if we see a buried assignment + while (true) + { + if (isa<CompoundStmt>(parent)) + break; + if (isa<CaseStmt>(parent) || isa<DefaultStmt>(parent) || isa<LabelStmt>(parent)) + return; + // Ignore assign in these statements, just seems to be part of the "natural" idiom of C/C++ + // TODO: perhaps include ReturnStmt? + if (auto forStmt = dyn_cast<ForStmt>(parent)) + { + if (child == forStmt->getBody()) + break; + return; + } + if (auto forRangeStmt = dyn_cast<CXXForRangeStmt>(parent)) + { + if (child == forRangeStmt->getBody()) + break; + return; + } + if (auto ifStmt = dyn_cast<IfStmt>(parent)) + { + if (child == ifStmt->getCond()) + return; + } + if (auto doStmt = dyn_cast<DoStmt>(parent)) + { + if (child == doStmt->getCond()) + return; + } + if (auto whileStmt = dyn_cast<WhileStmt>(parent)) + { + if (child == whileStmt->getBody()) + break; + return; + } + if (isa<ReturnStmt>(parent)) + return; + // This appears to be a valid way of making it obvious that we need to call acquire when assigning such ref-counted + // stuff e.g. + // rtl_uString_acquire( a = b ); + if (auto callExpr = dyn_cast<CallExpr>(parent)) + { + if (callExpr->getDirectCallee() && callExpr->getDirectCallee()->getIdentifier()) + { + auto name = callExpr->getDirectCallee()->getName(); + if (name == "rtl_uString_acquire" || name == "_acquire" + || name == "typelib_typedescriptionreference_acquire") + return; + } + } + child = parent; + parent = getParentStmt(parent); + if (!parent) + break; + } + + report(DiagnosticsEngine::Warning, "buried assignment, very hard to read", + compat::getBeginLoc(binaryOp)) + << binaryOp->getSourceRange(); +} + +// off by default because it uses getParentStmt so it's more expensive to run +loplugin::Plugin::Registration<BuriedAssign> X("buriedassign", false); +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/compilerplugins/clang/test/buriedassign.cxx b/compilerplugins/clang/test/buriedassign.cxx new file mode 100644 index 000000000000..7e41a83ccaa5 --- /dev/null +++ b/compilerplugins/clang/test/buriedassign.cxx @@ -0,0 +1,80 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */ +/* + * This file is part of the LibreOffice project. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +#include <map> +#include <rtl/ustring.hxx> + +namespace test1 +{ +int foo(int); + +void main() +{ + int x = 1; + foo(x = 2); // expected-error {{buried assignment, very hard to read [loplugin:buriedassign]}} + int y = x = 1; // no warning expected + (void)y; + int z = foo( + x = 1); // expected-error {{buried assignment, very hard to read [loplugin:buriedassign]}} + (void)z; + switch (x = 1) + { // expected-error@-1 {{buried assignment, very hard to read [loplugin:buriedassign]}} + } + std::map<int, int> map1; + map1[x = 1] + = 1; // expected-error@-1 {{buried assignment, very hard to read [loplugin:buriedassign]}} +} +} + +namespace test2 +{ +struct MyInt +{ + int x; + MyInt(int i) + : x(i) + { + } + MyInt& operator=(MyInt const&) = default; + MyInt& operator=(int) { return *this; } + bool operator<(MyInt const& other) const { return x < other.x; } +}; + +MyInt foo(MyInt); + +void main() +{ + MyInt x = 1; + foo(x = 2); // expected-error {{buried assignment, very hard to read [loplugin:buriedassign]}} + MyInt y = x = 1; // no warning expected + (void)y; + MyInt z = foo( + x = 1); // expected-error {{buried assignment, very hard to read [loplugin:buriedassign]}} + (void)z; + z = x; // no warning expected + std::map<MyInt, int> map1; + map1[x = 1] + = 1; // expected-error@-1 {{buried assignment, very hard to read [loplugin:buriedassign]}} +} +} + +namespace test3 +{ +void main(OUString sUserAutoCorrFile, OUString sExt) +{ + OUString sRet; + if (sUserAutoCorrFile == "xxx") + sRet = sUserAutoCorrFile; // no warning expected + if (sUserAutoCorrFile == "yyy") + (sRet = sUserAutoCorrFile) + += sExt; // expected-error@-1 {{buried assignment, very hard to read [loplugin:buriedassign]}} +} +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ |