[Libreoffice-commits] core.git: compilerplugins/clang
Noel Grandin
noel.grandin at collabora.co.uk
Tue Jun 20 05:50:08 UTC 2017
compilerplugins/clang/oncevar.cxx | 288 ++++++++++++++++++++++++++++++++
compilerplugins/clang/store/oncevar.cxx | 166 ------------------
compilerplugins/clang/test/oncevar.cxx | 54 ++++++
3 files changed, 342 insertions(+), 166 deletions(-)
New commits:
commit 9c2b43e86fbb7612a58f6e55bc429f674977d6dd
Author: Noel Grandin <noel.grandin at collabora.co.uk>
Date: Mon Jun 19 16:00:16 2017 +0200
improve oncevar loplugin
we look for any kind of scalar variable now that deserves to be inlined,
and we check for variables that cannot be inlined because they are being
passed by reference, or modified, or have their address taken
Change-Id: Ia744a180e91d1516140a1555d4514f6fa4de1c0b
Reviewed-on: https://gerrit.libreoffice.org/38966
Tested-by: Jenkins <ci at libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin at collabora.co.uk>
diff --git a/compilerplugins/clang/oncevar.cxx b/compilerplugins/clang/oncevar.cxx
new file mode 100644
index 000000000000..d6c73e32b84b
--- /dev/null
+++ b/compilerplugins/clang/oncevar.cxx
@@ -0,0 +1,288 @@
+/* -*- 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 <string>
+#include <iostream>
+#include <unordered_map>
+#include <unordered_set>
+
+#include "plugin.hxx"
+#include "check.hxx"
+#include "clang/AST/CXXInheritance.h"
+
+// Original idea from tml.
+// Look for variables that are (a) initialised from zero or one constants. (b) only used in one spot.
+// In which case, we might as well inline it.
+
+namespace
+{
+
+bool startsWith(const std::string& rStr, const char* pSubStr) {
+ return rStr.compare(0, strlen(pSubStr), pSubStr) == 0;
+}
+
+class OnceVar:
+ public RecursiveASTVisitor<OnceVar>, public loplugin::Plugin
+{
+public:
+ explicit OnceVar(InstantiationData const & data): Plugin(data) {}
+
+ virtual void run() override {
+ // ignore some files with problematic macros
+ std::string fn( compiler.getSourceManager().getFileEntryForID(
+ compiler.getSourceManager().getMainFileID())->getName() );
+ normalizeDotDotInFilePath(fn);
+ // platform-specific stuff
+ if (fn == SRCDIR "/sal/osl/unx/thread.cxx"
+ || fn == SRCDIR "/sot/source/base/formats.cxx"
+ || fn == SRCDIR "/svl/source/config/languageoptions.cxx"
+ || fn == SRCDIR "/sfx2/source/appl/appdde.cxx"
+ || fn == SRCDIR "/configmgr/source/components.cxx"
+ || fn == SRCDIR "/embeddedobj/source/msole/oleembed.cxx")
+ return;
+ // some of this is necessary
+ if (startsWith( fn, SRCDIR "/sal/qa/"))
+ return;
+ if (startsWith( fn, SRCDIR "/comphelper/qa/"))
+ return;
+ // TODO need to check calls via function pointer
+ if (fn == SRCDIR "/i18npool/source/textconversion/textconversion_zh.cxx"
+ || fn == SRCDIR "/i18npool/source/localedata/localedata.cxx")
+ return;
+ // debugging stuff
+ if (fn == SRCDIR "/sc/source/core/data/dpcache.cxx"
+ || fn == SRCDIR "/sw/source/core/layout/dbg_lay.cxx"
+ || fn == SRCDIR "/sw/source/core/layout/ftnfrm.cxx")
+ return;
+ // TODO taking local reference to variable
+ if (fn == SRCDIR "/sc/source/filter/excel/xechart.cxx")
+ return;
+ TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+
+ for (auto const & varDecl : maVarDeclSet)
+ {
+ if (maVarDeclToIgnoreSet.find(varDecl) != maVarDeclToIgnoreSet.end())
+ continue;
+ int noUses = 0;
+ auto it = maVarUsesMap.find(varDecl);
+ if (it != maVarUsesMap.end())
+ noUses = it->second;
+ if (noUses > 1)
+ continue;
+ report(DiagnosticsEngine::Warning,
+ "var used only once, should be inlined or declared const",
+ varDecl->getLocation())
+ << varDecl->getSourceRange();
+ if (it != maVarUsesMap.end())
+ report(DiagnosticsEngine::Note,
+ "used here",
+ maVarUseSourceRangeMap[varDecl].getBegin())
+ << maVarUseSourceRangeMap[varDecl];
+ }
+ }
+
+ bool VisitDeclRefExpr( const DeclRefExpr* );
+ bool VisitVarDecl( const VarDecl* );
+
+private:
+ StringRef getFilename(SourceLocation loc);
+
+ std::unordered_set<VarDecl const *> maVarDeclSet;
+ std::unordered_set<VarDecl const *> maVarDeclToIgnoreSet;
+ std::unordered_map<VarDecl const *, int> maVarUsesMap;
+ std::unordered_map<VarDecl const *, SourceRange> maVarUseSourceRangeMap;
+};
+
+StringRef OnceVar::getFilename(SourceLocation loc)
+{
+ SourceLocation spellingLocation = compiler.getSourceManager().getSpellingLoc(loc);
+ StringRef name { compiler.getSourceManager().getFilename(spellingLocation) };
+ return name;
+}
+
+bool OnceVar::VisitVarDecl( const VarDecl* varDecl )
+{
+ if (ignoreLocation(varDecl)) {
+ return true;
+ }
+ if (varDecl->isExceptionVariable() || isa<ParmVarDecl>(varDecl)) {
+ return true;
+ }
+ // ignore stuff in header files (which should really not be there, but anyhow)
+ if (!compiler.getSourceManager().isInMainFile(varDecl->getLocation())) {
+ return true;
+ }
+ // Ignore macros like FD_ZERO
+ if (compiler.getSourceManager().isMacroBodyExpansion(varDecl->getLocStart())) {
+ return true;
+ }
+ if (varDecl->hasGlobalStorage()) {
+ return true;
+ }
+ auto const tc = loplugin::TypeCheck(varDecl->getType());
+ if (!varDecl->getType()->isScalarType()
+ && !varDecl->getType()->isBooleanType()
+ && !varDecl->getType()->isEnumeralType()
+ && !tc.Class("OString").Namespace("rtl").GlobalNamespace()
+ && !tc.Class("OUString").Namespace("rtl").GlobalNamespace()
+ && !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace()
+ && !tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace())
+ {
+ return true;
+ }
+ if (varDecl->getType()->isPointerType())
+ return true;
+ // if it's declared const, ignore it, it's there to make the code easier to read
+ if (tc.Const())
+ return true;
+
+ if (!varDecl->hasInit())
+ return true;
+
+ // check for string or scalar literals
+ bool foundStringLiteral = false;
+ const Expr * initExpr = varDecl->getInit();
+ if (auto e = dyn_cast<ExprWithCleanups>(initExpr)) {
+ initExpr = e->getSubExpr();
+ }
+ if (auto stringLit = dyn_cast<clang::StringLiteral>(initExpr)) {
+ foundStringLiteral = true;
+ // ignore long literals, helps to make the code more legible
+ if (stringLit->getLength() > 40) {
+ return true;
+ }
+ } else if (auto constructExpr = dyn_cast<CXXConstructExpr>(initExpr)) {
+ if (constructExpr->getNumArgs() > 0) {
+ auto stringLit2 = dyn_cast<clang::StringLiteral>(constructExpr->getArg(0));
+ foundStringLiteral = stringLit2 != nullptr;
+ // ignore long literals, helps to make the code more legible
+ if (stringLit2 && stringLit2->getLength() > 40) {
+ return true;
+ }
+ }
+ }
+ if (!foundStringLiteral
+ && !varDecl->getInit()->isConstantInitializer(compiler.getASTContext(), false/*ForRef*/))
+ {
+ return true;
+ }
+
+ maVarDeclSet.insert(varDecl);
+
+ return true;
+}
+
+bool OnceVar::VisitDeclRefExpr( const DeclRefExpr* declRefExpr )
+{
+ if (ignoreLocation(declRefExpr)) {
+ return true;
+ }
+ const Decl* decl = declRefExpr->getDecl();
+ if (!isa<VarDecl>(decl) || isa<ParmVarDecl>(decl)) {
+ return true;
+ }
+ const VarDecl * varDecl = dyn_cast<VarDecl>(decl)->getCanonicalDecl();
+ // ignore stuff in header files (which should really not be there, but anyhow)
+ if (!compiler.getSourceManager().isInMainFile(varDecl->getLocation())) {
+ return true;
+ }
+
+ Stmt const * parent = parentStmt(declRefExpr);
+ // ignore cases like:
+ // const OUString("xxx") xxx;
+ // rtl_something(xxx.pData);
+ // and
+ // foo(&xxx);
+ // where we cannot inline the declaration.
+ auto const tc = loplugin::TypeCheck(varDecl->getType());
+ if (tc.Class("OUString").Namespace("rtl").GlobalNamespace()
+ && parent && (isa<MemberExpr>(parent) || isa<UnaryOperator>(parent)))
+ {
+ maVarDeclToIgnoreSet.insert(varDecl);
+ return true;
+ }
+
+ // if we take the address of it, or we modify it, ignore it
+ if (auto unaryOp = dyn_cast_or_null<UnaryOperator>(parent)) {
+ UnaryOperator::Opcode op = unaryOp->getOpcode();
+ if (op == UO_AddrOf || op == UO_PreInc || op == UO_PostInc
+ || op == UO_PreDec || op == UO_PostDec)
+ {
+ maVarDeclToIgnoreSet.insert(varDecl);
+ return true;
+ }
+ }
+
+ // if we assign it another value, or modify it, ignore it
+ if (auto binaryOp = dyn_cast_or_null<BinaryOperator>(parent)) {
+ if (binaryOp->getLHS() == declRefExpr)
+ {
+ BinaryOperator::Opcode op = binaryOp->getOpcode();
+ if (op == BO_Assign || op == BO_PtrMemD || op == BO_PtrMemI || op == BO_MulAssign
+ || op == BO_DivAssign || op == BO_RemAssign || op == BO_AddAssign
+ || op == BO_SubAssign || op == BO_ShlAssign || op == BO_ShrAssign
+ || op == BO_AndAssign || op == BO_XorAssign || op == BO_OrAssign)
+ {
+ maVarDeclToIgnoreSet.insert(varDecl);
+ return true;
+ }
+ }
+ }
+
+ // ignore those ones we are passing by reference
+ if (auto callExpr = dyn_cast_or_null<CallExpr>(parent)) {
+ const FunctionDecl* calleeFunctionDecl = callExpr->getDirectCallee();
+ if (calleeFunctionDecl) {
+ for (unsigned i = 0; i < callExpr->getNumArgs(); ++i) {
+ if (callExpr->getArg(i) == declRefExpr) {
+ if (i < calleeFunctionDecl->getNumParams()) {
+ QualType qt { calleeFunctionDecl->getParamDecl(i)->getType() };
+ if (loplugin::TypeCheck(qt).LvalueReference()) {
+ maVarDeclToIgnoreSet.insert(varDecl);
+ return true;
+ }
+ }
+ break;
+ }
+ }
+ }
+ }
+ // ignore those ones we are passing by reference
+ if (auto cxxConstructExpr = dyn_cast_or_null<CXXConstructExpr>(parent)) {
+ const CXXConstructorDecl* cxxConstructorDecl = cxxConstructExpr->getConstructor();
+ for (unsigned i = 0; i < cxxConstructExpr->getNumArgs(); ++i) {
+ if (cxxConstructExpr->getArg(i) == declRefExpr) {
+ if (i < cxxConstructorDecl->getNumParams()) {
+ QualType qt { cxxConstructorDecl->getParamDecl(i)->getType() };
+ if (loplugin::TypeCheck(qt).LvalueReference()) {
+ maVarDeclToIgnoreSet.insert(varDecl);
+ return true;
+ }
+ }
+ break;
+ }
+ }
+ return true;
+ }
+
+ if (maVarUsesMap.find(varDecl) == maVarUsesMap.end()) {
+ maVarUsesMap[varDecl] = 1;
+ maVarUseSourceRangeMap[varDecl] = declRefExpr->getSourceRange();
+ } else {
+ maVarUsesMap[varDecl]++;
+ }
+
+ return true;
+}
+
+loplugin::Plugin::Registration< OnceVar > X("oncevar", false);
+
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/compilerplugins/clang/store/oncevar.cxx b/compilerplugins/clang/store/oncevar.cxx
deleted file mode 100644
index 1947515cd749..000000000000
--- a/compilerplugins/clang/store/oncevar.cxx
+++ /dev/null
@@ -1,166 +0,0 @@
-/* -*- 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 <string>
-#include <iostream>
-#include <unordered_map>
-
-#include "plugin.hxx"
-#include "check.hxx"
-#include "clang/AST/CXXInheritance.h"
-
-// Idea from tml.
-// Check for OUString/char[] variables that are
-// (1) initialised from a string literal
-// (2) only used in one spot
-// In which case, we might as well inline it.
-
-namespace
-{
-
-class OnceVar:
- public RecursiveASTVisitor<OnceVar>, public loplugin::Plugin
-{
-public:
- explicit OnceVar(InstantiationData const & data): Plugin(data) {}
-
- virtual void run() override {
- // ignore some files with problematic macros
- std::string fn( compiler.getSourceManager().getFileEntryForID(
- compiler.getSourceManager().getMainFileID())->getName() );
- normalizeDotDotInFilePath(fn);
- // TODO not possible here, need to figure out how to ignore cases where we index
- // into the string
- if (fn == SRCDIR "/vcl/source/filter/ixpm/xpmread.cxx")
- return;
- if (fn == SRCDIR "/sc/source/filter/excel/xiescher.cxx")
- return;
- // all the constants are nicely lined up at the top of the file, seems
- // a pity to just inline a handful.
- if (fn == SRCDIR "/sc/source/ui/docshell/docsh.cxx")
- return;
- if (fn == SRCDIR "/sw/source/core/text/EnhancedPDFExportHelper.cxx")
- return;
- if (fn == SRCDIR "/svgio/source/svgreader/svgtoken.cxx")
- return;
- // TODO explicit length array
- if (fn == SRCDIR "/sal/qa/osl/file/osl_File.cxx")
- return;
-
- TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
-
- for (auto it = maVarUsesMap.cbegin(); it != maVarUsesMap.cend(); ++it)
- {
- if (it->second == 1)
- {
- report(DiagnosticsEngine::Warning,
- "string/char[] var used only once, should be inlined",
- it->first)
- << maVarDeclSourceRangeMap[it->first];
- report(DiagnosticsEngine::Note,
- "to this spot",
- maVarUseSourceRangeMap[it->first].getBegin())
- << maVarUseSourceRangeMap[it->first];
- }
- }
- }
-
- bool VisitDeclRefExpr( const DeclRefExpr* );
-
-private:
- StringRef getFilename(SourceLocation loc);
-
- struct SourceLocationHash
- {
- size_t operator()( SourceLocation const & sl ) const
- {
- return sl.getRawEncoding();
- }
- };
- std::unordered_map<SourceLocation, int, SourceLocationHash> maVarUsesMap;
- std::unordered_map<SourceLocation, SourceRange, SourceLocationHash> maVarDeclSourceRangeMap;
- std::unordered_map<SourceLocation, SourceRange, SourceLocationHash> maVarUseSourceRangeMap;
-};
-
-StringRef OnceVar::getFilename(SourceLocation loc)
-{
- SourceLocation spellingLocation = compiler.getSourceManager().getSpellingLoc(loc);
- StringRef name { compiler.getSourceManager().getFilename(spellingLocation) };
- return name;
-}
-
-bool OnceVar::VisitDeclRefExpr( const DeclRefExpr* declRefExpr )
-{
- if (ignoreLocation(declRefExpr)) {
- return true;
- }
- const Decl* decl = declRefExpr->getDecl();
- if (!isa<VarDecl>(decl) || isa<ParmVarDecl>(decl)) {
- return true;
- }
- const VarDecl * varDecl = dyn_cast<VarDecl>(decl)->getCanonicalDecl();
- // ignore stuff in header files (which should really not be there, but anyhow)
- if (!compiler.getSourceManager().isInMainFile(varDecl->getLocation())) {
- return true;
- }
- if (!varDecl->hasInit()) {
- return true;
- }
- if (const StringLiteral* stringLit = dyn_cast<StringLiteral>(varDecl->getInit())) {
- // ignore long literals, helps to make the code more legible
- if (stringLit->getLength() > 40) {
- return true;
- }
- // ok
- } else {
- const CXXConstructExpr* constructExpr = dyn_cast<CXXConstructExpr>(varDecl->getInit());
- if (!constructExpr || constructExpr->getNumArgs() != 1) {
- return true;
- }
- const StringLiteral* stringLit2 = dyn_cast<StringLiteral>(varDecl->getInit());
- if (!stringLit2) {
- return true;
- }
- // ignore long literals, helps to make the code more legible
- if (stringLit2->getLength() > 40) {
- return true;
- }
- }
- SourceLocation loc = varDecl->getLocation();
-
- // ignore cases like:
- // const OUString("xxx") xxx;
- // rtl_something(xxx.pData);
- // and
- // foo(&xxx);
- // where we cannot inline the declaration.
- auto const tc = loplugin::TypeCheck(varDecl->getType());
- if (tc.Class("OUString").Namespace("rtl").GlobalNamespace()
- && (isa<MemberExpr>(parentStmt(declRefExpr))
- || isa<UnaryOperator>(parentStmt(declRefExpr))))
- {
- maVarUsesMap[loc] = 2;
- return true;
- }
-
- if (maVarUsesMap.find(loc) == maVarUsesMap.end()) {
- maVarUsesMap[loc] = 1;
- maVarDeclSourceRangeMap[loc] = varDecl->getSourceRange();
- maVarUseSourceRangeMap[loc] = declRefExpr->getSourceRange();
- } else {
- maVarUsesMap[loc]++;
- }
- return true;
-}
-
-loplugin::Plugin::Registration< OnceVar > X("oncevar");
-
-}
-
-/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/compilerplugins/clang/test/oncevar.cxx b/compilerplugins/clang/test/oncevar.cxx
new file mode 100644
index 000000000000..214293f185e5
--- /dev/null
+++ b/compilerplugins/clang/test/oncevar.cxx
@@ -0,0 +1,54 @@
+/* -*- 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 <rtl/ustring.hxx>
+
+/*int foo() { return 1; }*/
+
+void call_value(int); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}}
+void call_const_ref(int const &); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}}
+void call_ref(int &); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}}
+void call_value(OUString); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}}
+void call_const_ref(OUString const &); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}}
+void call_ref(OUString &); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}}
+
+int main() {
+/* TODO
+ int i;
+ int x = 2;
+ if ( (i = foo()) == 0 ) {
+ x = 1;
+ }
+*/
+
+
+ int i1 = 2; // expected-error {{var used only once, should be inlined or declared const [loplugin:oncevar]}}
+ call_value(i1); // expected-note {{used here [loplugin:oncevar]}}
+ int i2 = 2; // expected-error {{var used only once, should be inlined or declared const [loplugin:oncevar]}}
+ call_const_ref(i2); // expected-note {{used here [loplugin:oncevar]}}
+
+ // don't expect warnings here
+ int i3;
+ call_ref(i3);
+ int const i4 = 2;
+ call_value(i4);
+
+ OUString s1("xxx"); // expected-error {{var used only once, should be inlined or declared const [loplugin:oncevar]}}
+ call_value(s1); // expected-note {{used here [loplugin:oncevar]}}
+ OUString s2("xxx"); // expected-error {{var used only once, should be inlined or declared const [loplugin:oncevar]}}
+ call_const_ref(s2); // expected-note {{used here [loplugin:oncevar]}}
+
+ // don't expect warnings here
+ OUString s3;
+ call_ref(s3);
+ OUString const s4("xxx");
+ call_value(s4);
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
More information about the Libreoffice-commits
mailing list