diff options
Diffstat (limited to 'compilerplugins/clang/passstuffbyref.cxx')
| -rw-r--r-- | compilerplugins/clang/passstuffbyref.cxx | 509 |
1 files changed, 509 insertions, 0 deletions
diff --git a/compilerplugins/clang/passstuffbyref.cxx b/compilerplugins/clang/passstuffbyref.cxx new file mode 100644 index 0000000000..549987e43b --- /dev/null +++ b/compilerplugins/clang/passstuffbyref.cxx @@ -0,0 +1,509 @@ +/* -*- 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 <set> +#include <iostream> + +#include "check.hxx" +#include "plugin.hxx" + +// Find places where various things are passed by value. +// It's not very efficient, because we generally end up copying it twice - once into the parameter and +// again into the destination. +// They should rather be passed by reference. +// +// Generally recommending lambda capture by-ref rather than by-copy is even more +// problematic than with function parameters, as a lambda instance can easily +// outlive a referenced variable. So once lambdas start to get used in more +// sophisticated ways than passing them into standard algorithms, this plugin's +// advice, at least for explicit captures, will need to be revisited. + +namespace { + +class PassStuffByRef: + public loplugin::FilteringPlugin<PassStuffByRef> +{ +public: + explicit PassStuffByRef(loplugin::InstantiationData const & data): FilteringPlugin(data), mbInsideFunctionDecl(false), mbFoundReturnValueDisqualifier(false) {} + + virtual void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); } + + // When warning about function params of primitive type that could be passed + // by value instead of by reference, make sure not to warn if the parameter + // is ever bound to a reference; on the one hand, this needs scaffolding in + // all Traverse*Decl functions (indirectly) derived from FunctionDecl; and + // on the other hand, use a hack of ignoring just the DeclRefExprs nested in + // LValueToRValue ImplicitCastExprs when determining whether a param is + // bound to a reference: + bool TraverseFunctionDecl(FunctionDecl * decl); + bool TraverseCXXMethodDecl(CXXMethodDecl * decl); + bool TraverseCXXConstructorDecl(CXXConstructorDecl * decl); + bool TraverseCXXDestructorDecl(CXXDestructorDecl * decl); + bool TraverseCXXConversionDecl(CXXConversionDecl * decl); + bool VisitFunctionDecl(const FunctionDecl * decl); + bool TraverseImplicitCastExpr(ImplicitCastExpr * expr); + bool VisitDeclRefExpr(const DeclRefExpr * expr); + + bool VisitReturnStmt(const ReturnStmt * ); + bool VisitVarDecl(const VarDecl * ); + +private: + template<typename T> bool traverseAnyFunctionDecl( + T * decl, bool (RecursiveASTVisitor::* fn)(T *)); + void checkParams(const FunctionDecl * functionDecl); + void checkReturnValue(const FunctionDecl * functionDecl, const CXXMethodDecl * methodDecl); + bool isPrimitiveConstRef(QualType type); + bool isReturnExprDisqualified(const Expr* expr); + + bool mbInsideFunctionDecl; + bool mbFoundReturnValueDisqualifier; + + struct FDecl { + std::set<ParmVarDecl const *> parms; + bool check = false; + }; + std::vector<FDecl> functionDecls_; +}; + +bool PassStuffByRef::TraverseFunctionDecl(FunctionDecl * decl) { + return traverseAnyFunctionDecl( + decl, &RecursiveASTVisitor::TraverseFunctionDecl); +} + +bool PassStuffByRef::TraverseCXXMethodDecl(CXXMethodDecl * decl) { + return traverseAnyFunctionDecl( + decl, &RecursiveASTVisitor::TraverseCXXMethodDecl); +} + +bool PassStuffByRef::TraverseCXXConstructorDecl(CXXConstructorDecl * decl) { + return traverseAnyFunctionDecl( + decl, &RecursiveASTVisitor::TraverseCXXConstructorDecl); +} + +bool PassStuffByRef::TraverseCXXDestructorDecl(CXXDestructorDecl * decl) { + return traverseAnyFunctionDecl( + decl, &RecursiveASTVisitor::TraverseCXXDestructorDecl); +} + +bool PassStuffByRef::TraverseCXXConversionDecl(CXXConversionDecl * decl) { + return traverseAnyFunctionDecl( + decl, &RecursiveASTVisitor::TraverseCXXConversionDecl); +} + +template<typename T> bool PassStuffByRef::traverseAnyFunctionDecl( + T * decl, bool (RecursiveASTVisitor::* fn)(T *)) +{ + if (ignoreLocation(decl)) { + return true; + } + if (decl->doesThisDeclarationHaveABody()) { + functionDecls_.emplace_back(); + } + bool ret = (this->*fn)(decl); + if (decl->doesThisDeclarationHaveABody()) { + assert(!functionDecls_.empty()); + if (functionDecls_.back().check) { + for (auto d: functionDecls_.back().parms) { + report( + DiagnosticsEngine::Warning, + ("passing primitive type param %0 by const &, rather pass" + " by value"), + d->getLocation()) + << d->getType() << d->getSourceRange(); + auto can = decl->getCanonicalDecl(); + if (can->getLocation() != decl->getLocation()) { + report( + DiagnosticsEngine::Note, "function is declared here:", + can->getLocation()) + << can->getSourceRange(); + } + } + } + functionDecls_.pop_back(); + } + return ret; +} + +bool PassStuffByRef::VisitFunctionDecl(const FunctionDecl * functionDecl) { + if (ignoreLocation(functionDecl)) { + return true; + } + if (functionDecl->isDeleted() + || functionDecl->isFunctionTemplateSpecialization()) + { + return true; + } + // only consider base declarations, not overridden ones, or we warn on methods that + // are overriding stuff from external libraries + const CXXMethodDecl * methodDecl = dyn_cast<CXXMethodDecl>(functionDecl); + if (methodDecl && methodDecl->size_overridden_methods() > 0) { + return true; + } + + checkParams(functionDecl); + checkReturnValue(functionDecl, methodDecl); + return true; +} + +bool PassStuffByRef::TraverseImplicitCastExpr(ImplicitCastExpr * expr) { + if (ignoreLocation(expr)) { + return true; + } + return + (expr->getCastKind() == CK_LValueToRValue + && (dyn_cast<DeclRefExpr>(expr->getSubExpr()->IgnoreParenImpCasts()) + != nullptr)) + || RecursiveASTVisitor::TraverseImplicitCastExpr(expr); +} + +bool PassStuffByRef::VisitDeclRefExpr(const DeclRefExpr * expr) { + if (ignoreLocation(expr)) { + return true; + } + auto d = dyn_cast<ParmVarDecl>(expr->getDecl()); + if (d == nullptr) { + return true; + } + for (auto & fd: functionDecls_) { + if (fd.parms.erase(d) == 1) { + break; + } + } + return true; +} + +void PassStuffByRef::checkParams(const FunctionDecl * functionDecl) { + // Only warn on the definition of the function: + if (!functionDecl->doesThisDeclarationHaveABody()) { + return; + } + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(functionDecl)) { + return; + } + // these functions are passed as parameters to another function + if (loplugin::DeclCheck(functionDecl).MemberFunction() + .Class("ShapeAttributeLayer").Namespace("internal") + .Namespace("slideshow").GlobalNamespace()) + { + return; + } + unsigned n = functionDecl->getNumParams(); + assert(!functionDecls_.empty()); + functionDecls_.back().check = true; + for (unsigned i = 0; i != n; ++i) { + const ParmVarDecl * pvDecl = functionDecl->getParamDecl(i); + auto const t = pvDecl->getType(); + if (isPrimitiveConstRef(t)) { + functionDecls_.back().parms.insert(pvDecl); + } + } +} + +static bool startswith(const std::string& rStr, const char* pSubStr) { + return rStr.compare(0, strlen(pSubStr), pSubStr) == 0; +} + +void PassStuffByRef::checkReturnValue(const FunctionDecl * functionDecl, const CXXMethodDecl * methodDecl) { + if (methodDecl && (methodDecl->isVirtual() || methodDecl->hasAttr<OverrideAttr>())) { + return; + } + if( !functionDecl->doesThisDeclarationHaveABody() + || functionDecl->isLateTemplateParsed()) + { + return; + } + + const QualType type = functionDecl->getReturnType().getDesugaredType(compiler.getASTContext()); + if (type->isReferenceType() || type->isIntegralOrEnumerationType() || type->isPointerType() + || type->isTemplateTypeParmType() || type->isDependentType() || type->isBuiltinType() + || type->isScalarType()) + { + return; + } + + // not sure if it's possible to modify these + if (isa<CXXConversionDecl>(functionDecl)) + return; + + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(functionDecl)) { + return; + } + + loplugin::DeclCheck dc(functionDecl); + // function is passed as parameter to another function + if (dc.Function("ImplColMonoFnc").Class("GDIMetaFile").GlobalNamespace() + || (dc.Function("darkColor").Class("SvxBorderLine").Namespace("editeng") + .GlobalNamespace()) + || (dc.MemberFunction().Class("Binding").Namespace("xforms") + .GlobalNamespace()) + || (dc.MemberFunction().Class("Model").Namespace("xforms") + .GlobalNamespace()) + || (dc.MemberFunction().Class("Submission").Namespace("xforms") + .GlobalNamespace()) + || (dc.Function("TopLeft").Class("SwRect").GlobalNamespace()) + || (dc.Function("ConvDicList_CreateInstance").GlobalNamespace()) + || (dc.Function("Create").Class("OUnoAutoPilot").Namespace("dbp").GlobalNamespace()) + || (dc.Function("Size_").Class("SwRect").GlobalNamespace())) + { + return; + } + // not sure how to exclude this yet, returns copy of one of it's params + if (dc.Function("sameDistColor").GlobalNamespace() + || dc.Function("sameColor").GlobalNamespace() + || (dc.Operator(OO_Call).Struct("StringIdentity").AnonymousNamespace() + .Namespace("pcr").GlobalNamespace()) + || (dc.Function("accumulate").Namespace("internal") + .Namespace("slideshow").GlobalNamespace()) + || (dc.Function("lerp").Namespace("internal").Namespace("slideshow") + .GlobalNamespace())) + return; + // depends on a define + if (dc.Function("GetSharedFileURL").Class("SfxObjectShell") + .GlobalNamespace()) { + return; + } + // hides a constructor + if (dc.Function("createNonOwningCopy").Class("SortedAutoCompleteStrings").Namespace("editeng") + .GlobalNamespace()) { + return; + } + // template function + if (dc.Function("convertItems").Class("ValueParser").Namespace("configmgr").GlobalNamespace() + || dc.Function("parseListValue").AnonymousNamespace().Namespace("configmgr").GlobalNamespace() + || dc.Function("parseSingleValue").AnonymousNamespace().Namespace("configmgr").GlobalNamespace() + || dc.Function("Create").Class("HandlerComponentBase").Namespace("pcr").GlobalNamespace() + || dc.Function("toAny").Struct("Convert").Namespace("detail").Namespace("comphelper").GlobalNamespace() + || dc.Function("makeAny").Namespace("utl").GlobalNamespace()) { + return; + } + if (startswith(type.getAsString(), "struct o3tl::strong_int")) { + return; + } + auto tc = loplugin::TypeCheck(functionDecl->getReturnType()); + // these functions are passed by function-pointer + if (functionDecl->getIdentifier() && functionDecl->getName() == "GetRanges" + && tc.Struct("WhichRangesContainer").GlobalNamespace()) + return; + // extremely simple class, might as well pass by value + if (tc.Class("Color")) { + return; + } + // extremely simple class, might as well pass by value + if (tc.Struct("TranslateId")) { + return; + } + if (tc.Class("span").Namespace("o3tl")) { + return; + } + + // functionDecl->dump(); + + mbInsideFunctionDecl = true; + mbFoundReturnValueDisqualifier = false; + TraverseStmt(functionDecl->getBody()); + mbInsideFunctionDecl = false; + + if (mbFoundReturnValueDisqualifier) + return; + + report( DiagnosticsEngine::Warning, + "rather return %0 by const& than by value, to avoid unnecessary copying", + functionDecl->getSourceRange().getBegin()) + << type.getAsString() << functionDecl->getSourceRange(); + + // display the location of the class member declaration so I don't have to search for it by hand + auto canonicalDecl = functionDecl->getCanonicalDecl(); + if (functionDecl != canonicalDecl) + { + report( DiagnosticsEngine::Note, + "decl here", + canonicalDecl->getSourceRange().getBegin()) + << canonicalDecl->getSourceRange(); + } + + //functionDecl->dump(); +} + +bool PassStuffByRef::VisitReturnStmt(const ReturnStmt * returnStmt) +{ + if (!mbInsideFunctionDecl) + return true; + const Expr* expr = dyn_cast<Expr>(*returnStmt->child_begin())->IgnoreParenImpCasts(); + + if (isReturnExprDisqualified(expr)) + mbFoundReturnValueDisqualifier = true; + + return true; +} + +/** + * Does a return expression disqualify this method from doing return by const & ? + */ +bool PassStuffByRef::isReturnExprDisqualified(const Expr* expr) +{ + while (true) + { + expr = expr->IgnoreParens(); +// expr->dump(); + if (auto implicitCast = dyn_cast<ImplicitCastExpr>(expr)) { + expr = implicitCast->getSubExpr(); + continue; + } + if (auto exprWithCleanups = dyn_cast<ExprWithCleanups>(expr)) { + expr = exprWithCleanups->getSubExpr(); + continue; + } + if (auto constructExpr = dyn_cast<CXXConstructExpr>(expr)) + { + if (constructExpr->getNumArgs()==1 + && constructExpr->getConstructor()->isCopyOrMoveConstructor()) + { + expr = constructExpr->getArg(0); + continue; + } + else + return true; + } + if (isa<CXXFunctionalCastExpr>(expr)) { + return true; + } + if (isa<MaterializeTemporaryExpr>(expr)) { + return true; + } + if (isa<CXXBindTemporaryExpr>(expr)) { + return true; + } + if (isa<InitListExpr>(expr)) { + return true; + } + expr = expr->IgnoreParenCasts(); + if (auto childExpr = dyn_cast<ArraySubscriptExpr>(expr)) { + expr = childExpr->getLHS(); + continue; + } + if (auto memberExpr = dyn_cast<MemberExpr>(expr)) { + expr = memberExpr->getBase(); + continue; + } + if (auto declRef = dyn_cast<DeclRefExpr>(expr)) { + // a param might be a temporary + if (isa<ParmVarDecl>(declRef->getDecl())) + return true; + const VarDecl* varDecl = dyn_cast<VarDecl>(declRef->getDecl()); + if (varDecl) { + if (varDecl->getStorageDuration() == SD_Thread + || varDecl->getStorageDuration() == SD_Static ) { + return false; + } + return true; + } + } + if (auto condOper = dyn_cast<ConditionalOperator>(expr)) { + return isReturnExprDisqualified(condOper->getTrueExpr()) + || isReturnExprDisqualified(condOper->getFalseExpr()); + } + if (auto unaryOp = dyn_cast<UnaryOperator>(expr)) { + expr = unaryOp->getSubExpr(); + continue; + } + if (auto operatorCallExpr = dyn_cast<CXXOperatorCallExpr>(expr)) { + // TODO could improve this, but sometimes it means we're returning a copy of a temporary. + // Same logic as CXXOperatorCallExpr::isAssignmentOp(), which our supported clang + // doesn't have yet. + auto Opc = operatorCallExpr->getOperator(); + if (Opc == OO_Equal || Opc == OO_StarEqual || + Opc == OO_SlashEqual || Opc == OO_PercentEqual || + Opc == OO_PlusEqual || Opc == OO_MinusEqual || + Opc == OO_LessLessEqual || Opc == OO_GreaterGreaterEqual || + Opc == OO_AmpEqual || Opc == OO_CaretEqual || + Opc == OO_PipeEqual) + return true; + if (Opc == OO_Subscript) + { + if (isReturnExprDisqualified(operatorCallExpr->getArg(0))) + return true; + // otherwise fall through to the checking below + } + if (Opc == OO_Arrow) + if (isReturnExprDisqualified(operatorCallExpr->getArg(0))) + return true; + } + if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(expr)) { + if (isReturnExprDisqualified(memberCallExpr->getImplicitObjectArgument())) + return true; + // otherwise fall through to the checking in CallExpr + } + if (auto callExpr = dyn_cast<CallExpr>(expr)) { + FunctionDecl const * calleeFunctionDecl = callExpr->getDirectCallee(); + if (!calleeFunctionDecl) + return true; + // TODO anything takes a non-integral param is suspect because it might return the param by ref. + // we could tighten this to only reject functions that have a param of the same type + // as the return type. Or we could check for such functions and disallow them. + // Or we could force such functions to be annotated somehow. + for (unsigned i = 0; i != calleeFunctionDecl->getNumParams(); ++i) { + if (!calleeFunctionDecl->getParamDecl(i)->getType()->isIntegralOrEnumerationType()) + return true; + } + auto tc = loplugin::TypeCheck(calleeFunctionDecl->getReturnType()); + if (!tc.LvalueReference() && !tc.Pointer()) + return true; + } + return false; + } +} + +bool PassStuffByRef::VisitVarDecl(const VarDecl * varDecl) +{ + if (!mbInsideFunctionDecl || mbFoundReturnValueDisqualifier) + return true; + // things guarded by locking are probably best left alone + loplugin::TypeCheck dc(varDecl->getType()); + if (dc.Class("Guard").Namespace("osl").GlobalNamespace() || + dc.Class("ClearableGuard").Namespace("osl").GlobalNamespace() || + dc.Class("ResettableGuard").Namespace("osl").GlobalNamespace() || + dc.Class("SolarMutexGuard").GlobalNamespace() || + dc.Class("SfxModelGuard").GlobalNamespace() || + dc.Class("ReadWriteGuard").Namespace("utl").GlobalNamespace() || + dc.Class("unique_lock").StdNamespace() || + dc.Class("lock_guard").StdNamespace() || + dc.Class("scoped_lock").StdNamespace()) + mbFoundReturnValueDisqualifier = true; + return true; +} + +bool PassStuffByRef::isPrimitiveConstRef(QualType type) { + if (type->isIncompleteType()) { + return false; + } + const clang::ReferenceType* referenceType = type->getAs<ReferenceType>(); + if (referenceType == nullptr) { + return false; + } + QualType pointeeQT = referenceType->getPointeeType(); + if (!pointeeQT.isConstQualified()) { + return false; + } + if (!pointeeQT->isFundamentalType()) { + return false; + } + // ignore double for now, some of our code seems to believe it is cheaper to pass by ref + const BuiltinType* builtinType = pointeeQT->getAs<BuiltinType>(); + return builtinType->getKind() != BuiltinType::Kind::Double; +} + + +loplugin::Plugin::Registration< PassStuffByRef > X("passstuffbyref", false); + +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ |