[Libreoffice-commits] core.git: compilerplugins/clang solenv/CompilerTest_compilerplugins_clang.mk solenv/gbuild
Stephan Bergmann
sbergman at redhat.com
Fri Apr 28 12:38:30 UTC 2017
compilerplugins/clang/cppunitassertequals.cxx | 142 ++++++++++++++-------
compilerplugins/clang/test/cppunitassertequals.cxx | 56 ++++++++
compilerplugins/clang/test/cppunitassertequals.hxx | 23 +++
solenv/CompilerTest_compilerplugins_clang.mk | 5
solenv/gbuild/CompilerTest.mk | 1
5 files changed, 183 insertions(+), 44 deletions(-)
New commits:
commit a528392e71bc70136021be4e3d83732fccbb885e
Author: Stephan Bergmann <sbergman at redhat.com>
Date: Fri Apr 28 14:23:35 2017 +0200
Fixed/improved loplugin:cppunitassertequals
* 994e38e336beeacbd983faafac480afc94d3947e "loplugin: use TypeCheck instead of
getQualifiedNameAsString" had effectively disabled this plugin (Asserter is a
struct, not a namespace). Fixed that.
* Also improved the checks, for one removing the---expensive---use of
Plugin::parentStmt, for another making the plugin look into (...), !..., and
...&&... expressions.
* However, as the plugin had effectively already been disabled (see above) when
it was switched on generally with 839e53b933322b739a7f534af58c63a2c69af7bd
"compilerplugins: enable loplugin:cppunitassertequals by default", it now hit
way more places than I had initially anticipated. To keep the amount of work
manageable, midway-through I disabled looking into ...&&... expressions for
now. That will be enabled (and resulting warnings fixed) in follow-up
commits.
* Checks like
CPPUNIT_ASSERT(a == b)
that actually want to check a specific overloaded operator == implementation,
rather than using such an operator == to actually check that a and b are
equal, can be rewritten as
CPPUNIT_ASSERT(operator ==(a, b))
to avoid false warnings from this plugin.
Change-Id: If3501020e2d150ad0f2454a65a39081e31470c0f
diff --git a/compilerplugins/clang/cppunitassertequals.cxx b/compilerplugins/clang/cppunitassertequals.cxx
index 858ad9615cbe..37dff6acca97 100644
--- a/compilerplugins/clang/cppunitassertequals.cxx
+++ b/compilerplugins/clang/cppunitassertequals.cxx
@@ -7,11 +7,6 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/
-#include <cassert>
-#include <string>
-#include <iostream>
-#include <fstream>
-#include <set>
#include "plugin.hxx"
#include "check.hxx"
@@ -29,74 +24,133 @@ public:
virtual void run() override
{
- TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+ if (compiler.getLangOpts().CPlusPlus) {
+ TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+ }
}
bool VisitCallExpr(const CallExpr*);
- bool VisitBinaryOperator(const BinaryOperator*);
+
private:
- void checkExpr(const Stmt* stmt);
+ void checkExpr(
+ SourceRange range, StringRef name, Expr const * expr, bool negated);
+
+ void reportEquals(SourceRange range, StringRef name, bool negative);
};
bool CppunitAssertEquals::VisitCallExpr(const CallExpr* callExpr)
{
- if (ignoreLocation(callExpr)) {
+ auto const decl = callExpr->getDirectCallee();
+ if (decl == nullptr
+ || !(loplugin::DeclCheck(decl).Function("failIf").Struct("Asserter")
+ .Namespace("CppUnit").GlobalNamespace()))
+ {
return true;
}
- if (callExpr->getDirectCallee() == nullptr) {
+ // Don't use callExpr->getLocStart() or callExpr->getExprLoc(), as those
+ // fall into a nested use of the CPPUNIT_NS macro; CallExpr::getRParenLoc
+ // happens to be readily available and cause good results:
+ auto loc = callExpr->getRParenLoc();
+ while (compiler.getSourceManager().isMacroArgExpansion(loc)) {
+ loc = compiler.getSourceManager().getImmediateMacroCallerLoc(loc);
+ }
+ if (!compiler.getSourceManager().isMacroBodyExpansion(loc)
+ || ignoreLocation(
+ compiler.getSourceManager().getImmediateMacroCallerLoc(loc)))
+ {
return true;
}
- const FunctionDecl* functionDecl = callExpr->getDirectCallee()->getCanonicalDecl();
- if (functionDecl->getOverloadedOperator() != OO_EqualEqual) {
+ auto name = Lexer::getImmediateMacroName(
+ loc, compiler.getSourceManager(), compiler.getLangOpts());
+ if (name != "CPPUNIT_ASSERT" && name != "CPPUNIT_ASSERT_MESSAGE") {
return true;
}
- checkExpr(callExpr);
- return true;
-}
-
-bool CppunitAssertEquals::VisitBinaryOperator(const BinaryOperator* binaryOp)
-{
- if (ignoreLocation(binaryOp)) {
+ if (decl->getNumParams() != 3) {
+ report(
+ DiagnosticsEngine::Warning,
+ ("TODO: suspicious CppUnit::Asserter::failIf call with %0"
+ " parameters"),
+ callExpr->getExprLoc())
+ << decl->getNumParams() << callExpr->getSourceRange();
return true;
}
- if (binaryOp->getOpcode() != BO_EQ) {
+ auto const e1 = callExpr->getArg(0)->IgnoreParenImpCasts();
+ Expr const * e2 = nullptr;
+ if (auto const e3 = dyn_cast<UnaryOperator>(e1)) {
+ if (e3->getOpcode() == UO_LNot) {
+ e2 = e3->getSubExpr();
+ }
+ } else if (auto const e4 = dyn_cast<CXXOperatorCallExpr>(e1)) {
+ if (e4->getOperator() == OO_Exclaim) {
+ e2 = e4->getArg(0);
+ }
+ }
+ if (e2 == nullptr) {
+ report(
+ DiagnosticsEngine::Warning,
+ ("TODO: suspicious CppUnit::Asserter::failIf call not wrapping"
+ " !(...)"),
+ callExpr->getExprLoc())
+ << callExpr->getSourceRange();
return true;
}
- checkExpr(binaryOp);
+ auto range = compiler.getSourceManager().getImmediateExpansionRange(loc);
+ checkExpr(
+ SourceRange(range.first, range.second), name,
+ e2->IgnoreParenImpCasts(), false);
return true;
}
-/*
- * check that we are not a compound expression
- */
-void CppunitAssertEquals::checkExpr(const Stmt* stmt)
+void CppunitAssertEquals::checkExpr(
+ SourceRange range, StringRef name, Expr const * expr, bool negated)
{
- const Stmt* parent = parentStmt(stmt);
- if (!parent || !isa<ParenExpr>(parent)) {
- return;
- }
- parent = parentStmt(parent);
- if (!parent || !isa<UnaryOperator>(parent)) {
- return;
- }
- parent = parentStmt(parent);
- if (!parent || !isa<CallExpr>(parent)) {
+ if (auto const e = dyn_cast<UnaryOperator>(expr)) {
+ if (e->getOpcode() == UO_LNot) {
+ checkExpr(
+ range, name, e->getSubExpr()->IgnoreParenImpCasts(), !negated);
+ }
return;
}
- const CallExpr* callExpr = dyn_cast<CallExpr>(parent);
- if (!callExpr || callExpr->getDirectCallee() == nullptr) {
+ if (auto const e = dyn_cast<BinaryOperator>(expr)) {
+ auto const op = e->getOpcode();
+ if ((!negated && op == BO_EQ) || (negated && op == BO_NE)) {
+ reportEquals(range, name, op == BO_NE);
+ return;
+ }
+#if 0 // TODO: enable later
+ if ((!negated && op == BO_LAnd) || (negated && op == BO_LOr)) {
+ report(
+ DiagnosticsEngine::Warning,
+ "rather split into two %0", e->getExprLoc())
+ << name << range;
+ return;
+ }
+#endif
return;
}
- const FunctionDecl* functionDecl = callExpr->getDirectCallee()->getCanonicalDecl();
- if (!functionDecl ||
- !loplugin::DeclCheck(functionDecl).Function("failIf").Namespace("Asserter").Namespace("CppUnit").GlobalNamespace())
- {
+ if (auto const e = dyn_cast<CXXOperatorCallExpr>(expr)) {
+ auto const op = e->getOperator();
+ if ((!negated && op == OO_EqualEqual)
+ || (negated && op == OO_ExclaimEqual))
+ {
+ reportEquals(range, name, op == OO_ExclaimEqual);
+ return;
+ }
return;
}
+}
+
+void CppunitAssertEquals::reportEquals(
+ SourceRange range, StringRef name, bool negative)
+{
report(
- DiagnosticsEngine::Warning, "rather call CPPUNIT_ASSERT_EQUALS",
- stmt->getSourceRange().getBegin())
- << stmt->getSourceRange();
+ DiagnosticsEngine::Warning,
+ ("rather call"
+ " %select{CPPUNIT_ASSERT_EQUAL|CPPUNIT_ASSERT_EQUAL_MESSAGE}0 (or"
+ " rewrite as an explicit operator %select{==|!=}1 call when the"
+ " operator itself is the topic)"),
+ range.getBegin())
+ << (name == "CPPUNIT_ASSERT_MESSAGE") << negative << range;
}
loplugin::Plugin::Registration< CppunitAssertEquals > X("cppunitassertequals");
diff --git a/compilerplugins/clang/test/cppunitassertequals.cxx b/compilerplugins/clang/test/cppunitassertequals.cxx
new file mode 100644
index 000000000000..239de58853b3
--- /dev/null
+++ b/compilerplugins/clang/test/cppunitassertequals.cxx
@@ -0,0 +1,56 @@
+/* -*- 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 "sal/config.h"
+
+#include "cppunit/TestAssert.h"
+
+#include "cppunitassertequals.hxx"
+
+#define TEST1 CPPUNIT_ASSERT(b1 == b2)
+#define TEST2(x) x
+
+void test(bool b1, bool b2, OUString const & s1, OUString const & s2, T t) {
+ CppUnit::Asserter::failIf(b1,"");
+#if 0 // TODO: enable later
+ CPPUNIT_ASSERT(b1 && b2); // expected-error {{rather split into two CPPUNIT_ASSERT [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT((b1 && b2)); // expected-error {{rather split into two CPPUNIT_ASSERT [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT(!(b1 || b2)); // expected-error {{rather split into two CPPUNIT_ASSERT [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT(!(b1 && b2));
+ CPPUNIT_ASSERT(!!(b1 && b2)); // expected-error {{rather split into two CPPUNIT_ASSERT [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT_MESSAGE("", b1 && b2); // expected-error {{rather split into two CPPUNIT_ASSERT_MESSAGE [loplugin:cppunitassertequals]}}
+#endif
+ CPPUNIT_ASSERT(b1 == b2); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT(b1 != b2);
+ CPPUNIT_ASSERT((b1 == b2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT(!(b1 != b2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator != call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT(!(b1 == b2));
+ CPPUNIT_ASSERT(!!(b1 == b2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT_MESSAGE("", b1 == b2); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL_MESSAGE (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT(s1 == s2); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT(s1 != s2);
+ CPPUNIT_ASSERT((s1 == s2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT(!(s1 != s2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator != call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ CPPUNIT_ASSERT(!(s1 == s2));
+ CPPUNIT_ASSERT(!!(s1 == s2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ TEST1; // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ TEST2(CPPUNIT_ASSERT(b1 == b2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+ TEST2(TEST1); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+
+ // Useful when testing an equality iterator itself:
+ CPPUNIT_ASSERT(operator ==(s1, s1));
+ CPPUNIT_ASSERT(t.operator ==(t));
+
+ // There might even be good reasons(?) not to warn inside explicit casts:
+ CPPUNIT_ASSERT(bool(b1 && b2));
+ CPPUNIT_ASSERT(bool(b1 == b2));
+ CPPUNIT_ASSERT(bool(s1 == s2));
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/test/cppunitassertequals.hxx b/compilerplugins/clang/test/cppunitassertequals.hxx
new file mode 100644
index 000000000000..b844c8387e67
--- /dev/null
+++ b/compilerplugins/clang/test/cppunitassertequals.hxx
@@ -0,0 +1,23 @@
+/* -*- 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/.
+ */
+
+#ifndef INCLUDED_COMPILERPLUGINS_CLANG_TEST_CPPUNITASSERTEQUALS_HXX
+#define INCLUDED_COMPILERPLUGINS_CLANG_TEST_CPPUNITASSERTEQUALS_HXX
+
+#include "sal/config.h"
+
+#include "rtl/ustring.hxx"
+
+struct T { bool operator ==(T); };
+
+void test(bool b1, bool b2, OUString const & s1, OUString const & s2, T t);
+
+#endif
+
+/* 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 724a9ac07046..9f8810eb4057 100644
--- a/solenv/CompilerTest_compilerplugins_clang.mk
+++ b/solenv/CompilerTest_compilerplugins_clang.mk
@@ -10,6 +10,7 @@
$(eval $(call gb_CompilerTest_CompilerTest,compilerplugins_clang))
$(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
+ compilerplugins/clang/test/cppunitassertequals \
compilerplugins/clang/test/datamembershadow \
compilerplugins/clang/test/externvar \
compilerplugins/clang/test/finalprotected \
@@ -27,6 +28,10 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
compilerplugins/clang/test/vclwidgets \
))
+$(eval $(call gb_CompilerTest_use_externals,compilerplugins_clang, \
+ cppunit \
+))
+
$(eval $(call gb_CompilerTest_use_udk_api,compilerplugins_clang))
# vim: set noet sw=4 ts=4:
diff --git a/solenv/gbuild/CompilerTest.mk b/solenv/gbuild/CompilerTest.mk
index a495cf368dda..e7f798ba0eae 100644
--- a/solenv/gbuild/CompilerTest.mk
+++ b/solenv/gbuild/CompilerTest.mk
@@ -42,6 +42,7 @@ $(eval $(foreach method, \
add_objcxxobjects \
add_cxxclrobject \
add_cxxclrobjects \
+ use_externals \
use_udk_api \
, \
$(call gb_CompilerTest__forward_to_Linktarget,$(method)) \
More information about the Libreoffice-commits
mailing list