compilerplugins/clang/passstuffbyref.cxx | 130 +++++++++++++++++++++++++++----
1 file changed, 114 insertions(+), 16 deletions(-)
New commits:
commit 8110dd24d11229b6518c8b2cd5289c20589e8258
Author: Stephan Bergmann <sberg...@redhat.com>
Date: Mon Jun 13 19:22:12 2016 +0200
Fix loplugin:passstuffbyref to not warn when ref param is bound to ref
cf. d150eab88ee26d5c05a6d662a2c13c6adea8ad78 "loplugin:passstuffbyref: For
now
disable 'pass parm by value' warnings". At least all the other changes in
4d49c9601c9b3e26a336e08e057d299895683480 "Let loplugin:passstuffbyref also
look
at fn defn not preceded by any decl" were OK but the one reverted with
b3e939971f56d53e60448a954a616ec295544098 "coverity#1362680 Pointer to local
outside scope".
Change-Id: I022125fbcb592e7da3c288c0fd09079dd2e87928
diff --git a/compilerplugins/clang/passstuffbyref.cxx
b/compilerplugins/clang/passstuffbyref.cxx
index 2768b81..d245152 100644
--- a/compilerplugins/clang/passstuffbyref.cxx
+++ b/compilerplugins/clang/passstuffbyref.cxx
@@ -35,25 +35,107 @@ public:
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 paremeter
+ // 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 VisitCallExpr(const CallExpr * ) { if (mbInsideFunctionDecl)
mbFoundDisqualifier = true; return true; }
bool VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr * ) { if
(mbInsideFunctionDecl) mbFoundDisqualifier = true; return true; }
bool VisitDeclStmt(const DeclStmt * ) { if (mbInsideFunctionDecl)
mbFoundDisqualifier = true; return true; }
bool VisitLambdaExpr(const LambdaExpr * expr);
private:
+ template<typename T> bool traverseAnyFunctionDecl(
+ T * decl, bool (RecursiveASTVisitor<PassStuffByRef>::* fn)(T *));
void checkParams(const FunctionDecl * functionDecl);
void checkReturnValue(const FunctionDecl * functionDecl, const
CXXMethodDecl * methodDecl);
bool isFat(QualType type);
bool isPrimitiveConstRef(QualType type);
bool mbInsideFunctionDecl;
bool mbFoundDisqualifier;
+
+ struct FDecl {
+ std::set<ParmVarDecl const *> parms;
+ bool check = false;
+ };
+ std::vector<FDecl> functionDecls_;
};
bool startswith(const std::string& rStr, const char* pSubStr) {
return rStr.compare(0, strlen(pSubStr), pSubStr) == 0;
}
+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<PassStuffByRef>::* 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;
@@ -75,9 +157,36 @@ bool PassStuffByRef::VisitFunctionDecl(const FunctionDecl *
functionDecl) {
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->isThisDeclarationADefinition()) {
+ if (!functionDecl->doesThisDeclarationHaveABody()) {
return;
}
unsigned n = functionDecl->getNumParams();
@@ -102,24 +211,13 @@ void PassStuffByRef::checkParams(const FunctionDecl *
functionDecl) {
if (startswith(aFunctionName, "slideshow::internal::ShapeAttributeLayer"))
{
return;
}
+ 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)) {
-#if 0 //TODO: check that the parm is not bound to a reference
- report(
- DiagnosticsEngine::Warning,
- ("passing primitive type param %0 by const &, rather pass by
value"),
- pvDecl->getLocation())
- << t << pvDecl->getSourceRange();
- auto can = functionDecl->getCanonicalDecl();
- if (can->getLocation() != functionDecl->getLocation()) {
- report(
- DiagnosticsEngine::Note, "function is declared here:",
- can->getLocation())
- << can->getSourceRange();
- }
-#endif
+ functionDecls_.back().parms.insert(pvDecl);
}
}
}
@@ -246,7 +344,7 @@ bool PassStuffByRef::isPrimitiveConstRef(QualType type) {
return false;
}
// ignore double for now, some of our code seems to believe it is cheaper
to pass by ref
- const BuiltinType* builtinType = dyn_cast<BuiltinType>(pointeeQT);
+ const BuiltinType* builtinType = pointeeQT->getAs<BuiltinType>();
return builtinType->getKind() != BuiltinType::Kind::Double;
}
_______________________________________________
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
