[Libreoffice-commits] core.git: compilerplugins/clang solenv/CompilerTest_compilerplugins_clang.mk
Libreoffice Gerrit user
logerrit at kemper.freedesktop.org
Fri Nov 16 13:20:30 UTC 2018
compilerplugins/clang/buriedassign.cxx | 226 +++++++++++++++++++++++++++
compilerplugins/clang/test/buriedassign.cxx | 80 +++++++++
solenv/CompilerTest_compilerplugins_clang.mk | 1
3 files changed, 307 insertions(+)
New commits:
commit faa63cd14c1d360d0e4eedb64cd879aa1229fa60
Author: Noel Grandin <noel.grandin at collabora.co.uk>
AuthorDate: Fri Nov 16 14:01:48 2018 +0200
Commit: Noel Grandin <noel.grandin at collabora.co.uk>
CommitDate: Fri Nov 16 14:20:04 2018 +0100
new loplugin buriedassign
Change-Id: If6dd8033daf2103a81c3a7c3a44cf1e38d0a3744
Reviewed-on: https://gerrit.libreoffice.org/63466
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin at collabora.co.uk>
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 at -1 {{buried assignment, very hard to read [loplugin:buriedassign]}}
+ }
+ std::map<int, int> map1;
+ map1[x = 1]
+ = 1; // expected-error at -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 at -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 at -1 {{buried assignment, very hard to read [loplugin:buriedassign]}}
+}
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/solenv/CompilerTest_compilerplugins_clang.mk b/solenv/CompilerTest_compilerplugins_clang.mk
index daea955e3135..5b07568c1f75 100644
--- a/solenv/CompilerTest_compilerplugins_clang.mk
+++ b/solenv/CompilerTest_compilerplugins_clang.mk
@@ -12,6 +12,7 @@ $(eval $(call gb_CompilerTest_CompilerTest,compilerplugins_clang))
$(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
compilerplugins/clang/test/badstatics \
compilerplugins/clang/test/blockblock \
+ compilerplugins/clang/test/buriedassign \
compilerplugins/clang/test/casttovoid \
compilerplugins/clang/test/collapseif \
compilerplugins/clang/test/commaoperator \
More information about the Libreoffice-commits
mailing list