diff options
Diffstat (limited to 'compilerplugins')
| -rw-r--r-- | compilerplugins/clang/constparams.cxx | 248 | ||||
| -rw-r--r-- | compilerplugins/clang/test/constparams.cxx | 27 |
2 files changed, 171 insertions, 104 deletions
diff --git a/compilerplugins/clang/constparams.cxx b/compilerplugins/clang/constparams.cxx index 76b1b91784a8..93b325a06657 100644 --- a/compilerplugins/clang/constparams.cxx +++ b/compilerplugins/clang/constparams.cxx @@ -8,7 +8,8 @@ */ #include <string> -#include <set> +#include <unordered_set> +#include <unordered_map> #include <iostream> #include "plugin.hxx" @@ -24,6 +25,10 @@ namespace { +static bool startswith(const std::string& rStr, const char* pSubStr) { + return rStr.compare(0, strlen(pSubStr), pSubStr) == 0; +} + class ConstParams: public RecursiveASTVisitor<ConstParams>, public loplugin::Plugin { @@ -31,35 +36,69 @@ public: explicit ConstParams(InstantiationData const & data): Plugin(data) {} virtual void run() override { + std::string fn( compiler.getSourceManager().getFileEntryForID( + compiler.getSourceManager().getMainFileID())->getName() ); + normalizeDotDotInFilePath(fn); + if (startswith(fn, SRCDIR "/sal/") + || startswith(fn, SRCDIR "/bridges/") + || startswith(fn, SRCDIR "/binaryurp/") + || startswith(fn, SRCDIR "/stoc/") + || startswith(fn, WORKDIR "/YaccTarget/unoidl/source/sourceprovider-parser.cxx") + // some weird calling through a function pointer + || startswith(fn, SRCDIR "/svtools/source/table/defaultinputhandler.cxx") + // windows only + || startswith(fn, SRCDIR "/basic/source/sbx/sbxdec.cxx") + || startswith(fn, SRCDIR "/sfx2/source/doc/syspath.cxx") + // ignore this for now + || startswith(fn, SRCDIR "/libreofficekit") + ) + return; + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + + for (const ParmVarDecl *pParmVarDecl : interestingSet) { + if (cannotBeConstSet.find(pParmVarDecl) == cannotBeConstSet.end()) { + report( + DiagnosticsEngine::Warning, + "this parameter can be const", + pParmVarDecl->getLocStart()) + << pParmVarDecl->getSourceRange(); + auto functionDecl = parmToFunction[pParmVarDecl]; + if (functionDecl->getCanonicalDecl()->getLocation() != functionDecl->getLocation()) { + unsigned idx = pParmVarDecl->getFunctionScopeIndex(); + const ParmVarDecl* pOther = functionDecl->getCanonicalDecl()->getParamDecl(idx); + report( + DiagnosticsEngine::Note, + "canonical parameter declaration here", + pOther->getLocStart()) + << pOther->getSourceRange(); + } + } + } } - bool VisitFunctionDecl(FunctionDecl *); - bool VisitDeclRefExpr( const DeclRefExpr* ); + bool VisitFunctionDecl(const FunctionDecl *); + bool VisitDeclRefExpr(const DeclRefExpr *); private: bool checkIfCanBeConst(const Stmt*, const ParmVarDecl*); bool isPointerOrReferenceToConst(const QualType& qt); StringRef getFilename(const SourceLocation& loc); - bool mbInsideFunction; - std::set<const ParmVarDecl*> interestingSet; - std::set<const ParmVarDecl*> cannotBeConstSet; + std::unordered_set<const ParmVarDecl*> interestingSet; + std::unordered_map<const ParmVarDecl*, const FunctionDecl*> parmToFunction; + std::unordered_set<const ParmVarDecl*> cannotBeConstSet; }; -bool ConstParams::VisitFunctionDecl(FunctionDecl * functionDecl) +bool ConstParams::VisitFunctionDecl(const FunctionDecl * functionDecl) { - if (ignoreLocation(functionDecl) || !functionDecl->doesThisDeclarationHaveABody()) { + if (ignoreLocation(functionDecl) || !functionDecl->isThisDeclarationADefinition()) { return true; } // ignore stuff that forms part of the stable URE interface if (isInUnoIncludeFile(functionDecl)) { return true; } - // TODO ignore these for now, requires some extra work - if (isa<CXXConstructorDecl>(functionDecl)) { - return true; - } // TODO ignore template stuff for now if (functionDecl->getTemplatedKind() != FunctionDecl::TK_NonTemplate) { return true; @@ -82,11 +121,12 @@ bool ConstParams::VisitFunctionDecl(FunctionDecl * functionDecl) if (functionDecl->getLocation().isMacroID()) return true; - if (functionDecl->getIdentifier()) { + if (functionDecl->getIdentifier()) + { StringRef name = functionDecl->getName(); if (name == "yyerror" // ignore lex/yacc callback - // some function callbacks - // TODO should really use a two-pass algorithm to detect and ignore these automatically + // some function callbacks + // TODO should really use a two-pass algorithm to detect and ignore these automatically || name == "PDFSigningPKCS7PasswordCallback" || name == "VCLExceptionSignal_impl" || name == "parseXcsFile" @@ -114,6 +154,7 @@ bool ConstParams::VisitFunctionDecl(FunctionDecl * functionDecl) || name == "ImpGetEscDir" || name == "ImpGetPercent" || name == "ImpGetAlign" + || name == "write_function" // #ifdef win32 || name == "convert_slashes" // UNO component entry points @@ -129,22 +170,7 @@ bool ConstParams::VisitFunctionDecl(FunctionDecl * functionDecl) return true; } - StringRef aFileName = getFilename(functionDecl->getLocStart()); - if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sal/") - || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/bridges/") - || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/binaryurp/") - || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/stoc/") - || loplugin::hasPathnamePrefix(aFileName, WORKDIR "/YaccTarget/unoidl/source/sourceprovider-parser.cxx") - // some weird calling through a function pointer - || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/svtools/source/table/defaultinputhandler.cxx") - // windows only - || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/basic/source/sbx/sbxdec.cxx") - || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sfx2/source/doc/syspath.cxx")) { - return true; - } - // calculate the ones we want to check - interestingSet.clear(); for (const ParmVarDecl *pParmVarDecl : compat::parameters(*functionDecl)) { // ignore unused params if (pParmVarDecl->getName().empty()) @@ -165,48 +191,27 @@ bool ConstParams::VisitFunctionDecl(FunctionDecl * functionDecl) // const is meaningless when applied to function pointer types if (pParmVarDecl->getType()->isFunctionPointerType()) continue; + // ignore things with template params + if (pParmVarDecl->getType()->isInstantiationDependentType()) + continue; interestingSet.insert(pParmVarDecl); + parmToFunction[pParmVarDecl] = functionDecl; } - if (interestingSet.empty()) { - return true; - } - - mbInsideFunction = true; - cannotBeConstSet.clear(); - bool ret = RecursiveASTVisitor::TraverseStmt(functionDecl->getBody()); - mbInsideFunction = false; - for (const ParmVarDecl *pParmVarDecl : interestingSet) { - if (cannotBeConstSet.find(pParmVarDecl) == cannotBeConstSet.end()) { - report( - DiagnosticsEngine::Warning, - "this parameter can be const", - pParmVarDecl->getLocStart()) - << pParmVarDecl->getSourceRange(); - if (functionDecl->getCanonicalDecl()->getLocation() != functionDecl->getLocation()) { - unsigned idx = pParmVarDecl->getFunctionScopeIndex(); - ParmVarDecl* pOther = functionDecl->getCanonicalDecl()->getParamDecl(idx); - report( - DiagnosticsEngine::Note, - "canonical parameter declaration here", - pOther->getLocStart()) - << pOther->getSourceRange(); - } - } - } - return ret; + return true; } bool ConstParams::VisitDeclRefExpr( const DeclRefExpr* declRefExpr ) { - if (!mbInsideFunction) { + if (ignoreLocation(declRefExpr)) { return true; } - const ParmVarDecl* parmVarDecl = dyn_cast_or_null<ParmVarDecl>(declRefExpr->getDecl()); - if (!parmVarDecl) { + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(declRefExpr->getLocStart())) { return true; } - if (interestingSet.find(parmVarDecl) == interestingSet.end()) { + const ParmVarDecl* parmVarDecl = dyn_cast_or_null<ParmVarDecl>(declRefExpr->getDecl()); + if (!parmVarDecl) { return true; } // no need to check again if we have already eliminated this one @@ -218,13 +223,41 @@ bool ConstParams::VisitDeclRefExpr( const DeclRefExpr* declRefExpr ) return true; } -// Walk up from a DeclRefExpr to a ParamVarDecl, checking if the usage means that the -// ParamVarDecl can be const. +// Walk up from a statement that contains a DeclRefExpr, checking if the usage means that the +// related ParamVarDecl can be const. bool ConstParams::checkIfCanBeConst(const Stmt* stmt, const ParmVarDecl* parmVarDecl) { const Stmt* parent = parentStmt( stmt ); - if (isa<UnaryOperator>(parent)) { - const UnaryOperator* unaryOperator = dyn_cast<UnaryOperator>(parent); + if (!parent) + { + // check if we're inside a CXXCtorInitializer + auto parentsRange = compiler.getASTContext().getParents(*stmt); + if ( parentsRange.begin() == parentsRange.end()) + return true; + auto cxxConstructorDecl = dyn_cast_or_null<CXXConstructorDecl>(parentsRange.begin()->get<Decl>()); + if (!cxxConstructorDecl) + return true; + for ( auto cxxCtorInitializer : cxxConstructorDecl->inits()) + { + if (cxxCtorInitializer->isAnyMemberInitializer() && cxxCtorInitializer->getInit() == stmt) + { + // if the member is not pointer or ref to-const, we cannot make the param const + auto fieldDecl = cxxCtorInitializer->getAnyMember(); + auto tc = loplugin::TypeCheck(fieldDecl->getType()); + return tc.Pointer().Const() || tc.LvalueReference().Const(); + } + } + parmVarDecl->dump(); + stmt->dump(); + cxxConstructorDecl->dump(); + report( + DiagnosticsEngine::Warning, + "couldn't find the CXXCtorInitializer?", + stmt->getLocStart()) + << stmt->getSourceRange(); + return false; + } + if (auto unaryOperator = dyn_cast<UnaryOperator>(parent)) { UnaryOperator::Opcode op = unaryOperator->getOpcode(); if (op == UO_AddrOf || op == UO_PreInc || op == UO_PostInc || op == UO_PreDec || op == UO_PostDec) { @@ -254,17 +287,15 @@ bool ConstParams::checkIfCanBeConst(const Stmt* stmt, const ParmVarDecl* parmVar return checkIfCanBeConst(parent, parmVarDecl); } return true; - } else if (isa<CXXConstructExpr>(parent)) { - const CXXConstructExpr* constructExpr = dyn_cast<CXXConstructExpr>(parent); + } else if (auto constructExpr = dyn_cast<CXXConstructExpr>(parent)) { const CXXConstructorDecl * constructorDecl = constructExpr->getConstructor(); for (unsigned i = 0; i < constructExpr->getNumArgs(); ++i) { if (constructExpr->getArg(i) == stmt) { return isPointerOrReferenceToConst(constructorDecl->getParamDecl(i)->getType()); } } - } else if (isa<CXXOperatorCallExpr>(parent)) { - const CXXOperatorCallExpr* operatorCallExpr = dyn_cast<CXXOperatorCallExpr>(parent); - const CXXMethodDecl* calleeMethodDecl = dyn_cast<CXXMethodDecl>(operatorCallExpr->getDirectCallee()); + } else if (auto operatorCallExpr = dyn_cast<CXXOperatorCallExpr>(parent)) { + const CXXMethodDecl* calleeMethodDecl = dyn_cast_or_null<CXXMethodDecl>(operatorCallExpr->getDirectCallee()); if (calleeMethodDecl) { // unary operator if (calleeMethodDecl->getNumParams() == 0) { @@ -293,8 +324,7 @@ bool ConstParams::checkIfCanBeConst(const Stmt* stmt, const ParmVarDecl* parmVar } } } - } else if (isa<CallExpr>(parent)) { - const CallExpr* callExpr = dyn_cast<CallExpr>(parent); + } else if (auto callExpr = dyn_cast<CallExpr>(parent)) { QualType functionType = callExpr->getCallee()->getType(); if (functionType->isFunctionPointerType()) { functionType = functionType->getPointeeType(); @@ -314,27 +344,29 @@ bool ConstParams::checkIfCanBeConst(const Stmt* stmt, const ParmVarDecl* parmVar } } const FunctionDecl* calleeFunctionDecl = callExpr->getDirectCallee(); - if (isa<CXXMemberCallExpr>(parent)) { - const CXXMemberCallExpr* memberCallExpr = dyn_cast<CXXMemberCallExpr>(parent); - const MemberExpr* memberExpr = dyn_cast<MemberExpr>(stmt); - if (memberExpr && memberCallExpr->getImplicitObjectArgument() == memberExpr->getBase()) - { - const CXXMethodDecl* calleeMethodDecl = dyn_cast<CXXMethodDecl>(calleeFunctionDecl); - return calleeMethodDecl->isConst(); + if (calleeFunctionDecl) + { + if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(parent)) { + const MemberExpr* memberExpr = dyn_cast<MemberExpr>(stmt); + if (memberExpr && memberCallExpr->getImplicitObjectArgument() == memberExpr->getBase()) + { + const CXXMethodDecl* calleeMethodDecl = dyn_cast<CXXMethodDecl>(calleeFunctionDecl); + return calleeMethodDecl->isConst(); + } } - } - // TODO could do better - if (calleeFunctionDecl->isVariadic()) { - return false; - } - if (callExpr->getCallee() == stmt) { - return true; - } - for (unsigned i = 0; i < callExpr->getNumArgs(); ++i) { - if (i >= calleeFunctionDecl->getNumParams()) // can happen in template code - break; - if (callExpr->getArg(i) == stmt) { - return isPointerOrReferenceToConst(calleeFunctionDecl->getParamDecl(i)->getType()); + // TODO could do better + if (calleeFunctionDecl->isVariadic()) { + return false; + } + if (callExpr->getCallee() == stmt) { + return true; + } + for (unsigned i = 0; i < callExpr->getNumArgs(); ++i) { + if (i >= calleeFunctionDecl->getNumParams()) // can happen in template code + break; + if (callExpr->getArg(i) == stmt) { + return isPointerOrReferenceToConst(calleeFunctionDecl->getParamDecl(i)->getType()); + } } } } else if (isa<CXXReinterpretCastExpr>(parent)) { @@ -370,15 +402,20 @@ bool ConstParams::checkIfCanBeConst(const Stmt* stmt, const ParmVarDecl* parmVar return false; } else if (isa<VAArgExpr>(parent)) { return false; + } else if (isa<CXXDependentScopeMemberExpr>(parent)) { + return false; } else if (isa<MaterializeTemporaryExpr>(parent)) { return true; - } else if (const ConditionalOperator* conditionalExpr = dyn_cast<ConditionalOperator>(parent)) { + } else if (auto conditionalExpr = dyn_cast<ConditionalOperator>(parent)) { if (conditionalExpr->getCond() == stmt) return true; return checkIfCanBeConst(parent, parmVarDecl); } else if (isa<UnaryExprOrTypeTraitExpr>(parent)) { return false; // ??? - } else if (isa<CXXNewExpr>(parent)) { + } else if (auto cxxNewExpr = dyn_cast<CXXNewExpr>(parent)) { + for (auto pa : cxxNewExpr->placement_arguments()) + if (pa == stmt) + return false; return true; // because the ParamVarDecl must be a parameter to the expression, probably an array length } else if (auto lambdaExpr = dyn_cast<LambdaExpr>(parent)) { for (auto it = lambdaExpr->capture_begin(); it != lambdaExpr->capture_end(); ++it) @@ -386,18 +423,21 @@ bool ConstParams::checkIfCanBeConst(const Stmt* stmt, const ParmVarDecl* parmVar if (it->capturesVariable() && it->getCapturedVar() == parmVarDecl) return it->getCaptureKind() != LCK_ByRef; } - /* sigh. just running this message will cause clang to crash (in sdext) - report( - DiagnosticsEngine::Warning, - "cannot handle this lambda", - parent->getLocStart()) - << parent->getSourceRange(); - parent->dump(); - parmVarDecl->dump(); - */ - return false; + return false; } else if (isa<CXXTypeidExpr>(parent)) { return true; + } else if (isa<ParenListExpr>(parent)) { + return true; + } else if (isa<CXXUnresolvedConstructExpr>(parent)) { + return false; + } else if (isa<UnresolvedMemberExpr>(parent)) { + return false; + } else if (isa<PackExpansionExpr>(parent)) { + return false; + } else if (isa<ExprWithCleanups>(parent)) { + return checkIfCanBeConst(parent, parmVarDecl); + } else if (isa<CaseStmt>(parent)) { + return true; } else { parent->dump(); parmVarDecl->dump(); diff --git a/compilerplugins/clang/test/constparams.cxx b/compilerplugins/clang/test/constparams.cxx new file mode 100644 index 000000000000..fd4ee2cd63f7 --- /dev/null +++ b/compilerplugins/clang/test/constparams.cxx @@ -0,0 +1,27 @@ +/* -*- 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/. + */ + +struct Class1 +{ + int const * m_f1; + Class1(int * f1) : m_f1(f1) {} // expected-error {{this parameter can be const [loplugin:constparams]}} +}; + +struct Class2 +{ + int * m_f2; + Class2(int * f2) : m_f2(f2) {} +}; +struct Class3 +{ + int * m_f2; + Class3(void * f2) : m_f2(static_cast<int*>(f2)) {} +}; + +/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ |