From 67af00a76a3bb86790447e2b223b03c5dddacc4a Mon Sep 17 00:00:00 2001 From: Noel Grandin Date: Fri, 7 Apr 2017 14:45:20 +0200 Subject: new loplugin:inlinefields look for fields which can be declared inline in the parent class. start with some likely candidates in svx Change-Id: I56cdca273272b72bb728ed2e3f5e1e976f8c7c32 Reviewed-on: https://gerrit.libreoffice.org/36262 Tested-by: Jenkins Reviewed-by: Noel Grandin --- compilerplugins/clang/inlinefields.cxx | 247 +++++++++++++++++++++++++++++++++ compilerplugins/clang/inlinefields.py | 81 +++++++++++ 2 files changed, 328 insertions(+) create mode 100644 compilerplugins/clang/inlinefields.cxx create mode 100755 compilerplugins/clang/inlinefields.py (limited to 'compilerplugins') diff --git a/compilerplugins/clang/inlinefields.cxx b/compilerplugins/clang/inlinefields.cxx new file mode 100644 index 000000000000..e663c118ed22 --- /dev/null +++ b/compilerplugins/clang/inlinefields.cxx @@ -0,0 +1,247 @@ +/* -*- 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 +#include +#include +#include +#include +#include "plugin.hxx" +#include "compat.hxx" + +/** +if a field is +- a pointer +- only assigned to in the constructor via a new expression +- unconditionally deleted in the destructor +then it can probably just be allocated inline in the parent object + +TODO check for cases where the pointer is passed by non-const reference + +Be warned that it produces around 5G of log file. + +The process goes something like this: + $ make check + $ make FORCE_COMPILE_ALL=1 COMPILER_PLUGIN_TOOL='inlinefields' check + $ ./compilerplugins/clang/inlinefields.py + +and then + $ for dir in *; do make FORCE_COMPILE_ALL=1 UPDATE_FILES=$dir COMPILER_PLUGIN_TOOL='inlinefieldsremove' $dir; done +to auto-remove the method declarations + +Note that the actual process may involve a fair amount of undoing, hand editing, and general messing around +to get it to work :-) + +*/ + +namespace { + +struct MyFieldInfo +{ + std::string parentClass; + std::string fieldName; + std::string sourceLocation; +}; +bool operator < (const MyFieldInfo &lhs, const MyFieldInfo &rhs) +{ + return std::tie(lhs.parentClass, lhs.fieldName) + < std::tie(rhs.parentClass, rhs.fieldName); +} + + +// try to limit the voluminous output a little +static std::set excludedSet; +static std::set definitionSet; +static std::set deletedInDestructorSet; +static std::set newedInConstructorSet; + +class InlineFields: + public RecursiveASTVisitor, public loplugin::Plugin +{ +public: + explicit InlineFields(InstantiationData const & data): Plugin(data) {} + + virtual void run() override + { + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + + // dump all our output in one write call - this is to try and limit IO "crosstalk" between multiple processes + // writing to the same logfile + std::string output; + for (const MyFieldInfo & s : definitionSet) + output += "definition:\t" + s.parentClass + "\t" + s.fieldName + "\t" + s.sourceLocation + "\n"; + for (const MyFieldInfo & s : excludedSet) + output += "excluded:\t" + s.parentClass + "\t" + s.fieldName + "\n"; + for (const MyFieldInfo & s : deletedInDestructorSet) + output += "deletedInDestructor:\t" + s.parentClass + "\t" + s.fieldName + "\n"; + for (const MyFieldInfo & s : newedInConstructorSet) + output += "newedInConstructor:\t" + s.parentClass + "\t" + s.fieldName + "\n"; + ofstream myfile; + myfile.open( SRCDIR "/loplugin.inlinefields.log", ios::app | ios::out); + myfile << output; + myfile.close(); + } + + bool shouldVisitTemplateInstantiations () const { return true; } + bool shouldVisitImplicitCode() const { return true; } + + bool VisitFieldDecl( const FieldDecl* ); + bool VisitCXXConstructorDecl( const CXXConstructorDecl* ); + bool VisitCXXDeleteExpr( const CXXDeleteExpr* ); + bool VisitBinAssign( const BinaryOperator* ); +private: + MyFieldInfo niceName(const FieldDecl*); + void checkTouched(const FieldDecl* fieldDecl, const Expr* memberExpr); +}; + +MyFieldInfo InlineFields::niceName(const FieldDecl* fieldDecl) +{ + MyFieldInfo aInfo; + + const RecordDecl* recordDecl = fieldDecl->getParent(); + + if (const CXXRecordDecl* cxxRecordDecl = dyn_cast(recordDecl)) + { + if (cxxRecordDecl->getTemplateInstantiationPattern()) + cxxRecordDecl = cxxRecordDecl->getTemplateInstantiationPattern(); + aInfo.parentClass = cxxRecordDecl->getQualifiedNameAsString(); + } + else + aInfo.parentClass = recordDecl->getQualifiedNameAsString(); + + aInfo.fieldName = fieldDecl->getNameAsString(); + + SourceLocation expansionLoc = compiler.getSourceManager().getExpansionLoc( fieldDecl->getLocation() ); + StringRef name = compiler.getSourceManager().getFilename(expansionLoc); + aInfo.sourceLocation = std::string(name.substr(strlen(SRCDIR)+1)) + ":" + std::to_string(compiler.getSourceManager().getSpellingLineNumber(expansionLoc)); + normalizeDotDotInFilePath(aInfo.sourceLocation); + + return aInfo; +} + +bool InlineFields::VisitFieldDecl( const FieldDecl* fieldDecl ) +{ + fieldDecl = fieldDecl->getCanonicalDecl(); + if (ignoreLocation( fieldDecl )) { + return true; + } + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocation()))) { + return true; + } + QualType type = fieldDecl->getType(); + if (!type->isPointerType()) + return true; + definitionSet.insert(niceName(fieldDecl)); + return true; +} + +bool InlineFields::VisitCXXConstructorDecl( const CXXConstructorDecl* decl ) +{ + if( ignoreLocation( decl ) ) + return true; + if (decl->isCopyOrMoveConstructor()) + return true; + for(auto it = decl->init_begin(); it != decl->init_end(); ++it) + { + const CXXCtorInitializer* init = *it; + const FieldDecl* fieldDecl = init->getMember(); + if( !fieldDecl || !fieldDecl->getType()->isPointerType() ) + continue; + auto e = init->getInit(); + if (auto parentListExpr = dyn_cast(e)) + e = parentListExpr->getExpr(0); + e = e->IgnoreParenImpCasts(); + if( isa(e) ) + newedInConstructorSet.insert(niceName(fieldDecl)); + else if( isa(e) || isa(e)) + ; // ignore + else + excludedSet.insert(niceName(fieldDecl)); + } + return true; +} + +static bool isSameParent(const CXXMethodDecl* cxxMethodDecl, const FieldDecl* fieldDecl) +{ + return cxxMethodDecl->getParent() == dyn_cast(fieldDecl->getParent()); +} + +bool InlineFields::VisitBinAssign(const BinaryOperator * binaryOp) +{ + if( ignoreLocation( binaryOp ) ) + return true; + auto memberExpr = dyn_cast(binaryOp->getLHS()); + if (!memberExpr) + return true; + auto fieldDecl = dyn_cast(memberExpr->getMemberDecl()); + if (!fieldDecl || !fieldDecl->getType()->isPointerType()) { + return true; + } + const FunctionDecl* parentFunction = parentFunctionDecl(binaryOp); + if (!parentFunction) { + return true; + } + // if the field is being assigned from outside it's own constructor or destructor, exclude + auto constructorDecl = dyn_cast(parentFunction); + if (constructorDecl && isSameParent(constructorDecl, fieldDecl)) { + if( isa(binaryOp->getRHS()) ) + newedInConstructorSet.insert(niceName(fieldDecl)); + else { + excludedSet.insert(niceName(fieldDecl)); + std::cout << "assign in constructor:" << std::endl; + binaryOp->getRHS()->dump(); + } + return true; + } + auto destructorDecl = dyn_cast(parentFunction); + if (destructorDecl && isSameParent(destructorDecl, fieldDecl)) { + auto e = binaryOp->getRHS()->IgnoreParenImpCasts(); + if( !isa(e) && !isa(e)) { + excludedSet.insert(niceName(fieldDecl)); + std::cout << "assign in destructor:" << std::endl; + e->dump(); + } + return true; + } + excludedSet.insert(niceName(fieldDecl)); + return true; +} + +bool InlineFields::VisitCXXDeleteExpr(const CXXDeleteExpr * deleteExpr) +{ + if( ignoreLocation( deleteExpr ) ) + return true; + auto memberExpr = dyn_cast(deleteExpr->getArgument()->IgnoreParenImpCasts()); + if (!memberExpr) + return true; + auto fieldDecl = dyn_cast(memberExpr->getMemberDecl()); + if (!fieldDecl || !fieldDecl->getType()->isPointerType()) { + return true; + } + // TODO for some reason, this part is not working properly, it doesn't find the parent + // function for delete statements properly + const FunctionDecl* parentFunction = parentFunctionDecl(deleteExpr); + if (!parentFunction) { + return true; + } + auto destructorDecl = dyn_cast(parentFunction); + if (destructorDecl && isSameParent(destructorDecl, fieldDecl)) { + deletedInDestructorSet.insert(niceName(fieldDecl)); + return true; + } + excludedSet.insert(niceName(fieldDecl)); + return true; +} + +loplugin::Plugin::Registration< InlineFields > X("inlinefields", false); + +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/compilerplugins/clang/inlinefields.py b/compilerplugins/clang/inlinefields.py new file mode 100755 index 000000000000..2605fd7bc173 --- /dev/null +++ b/compilerplugins/clang/inlinefields.py @@ -0,0 +1,81 @@ +#!/usr/bin/python + +import sys +import re +import io + +definitionToSourceLocationMap = dict() # dict of tuple(parentClass, fieldName) to sourceLocation +definitionSet = set() +excludedSet = set() +deletedInDestructorSet = set() +newedInConstructorSet = set(); + +# clang does not always use exactly the same numbers in the type-parameter vars it generates +# so I need to substitute them to ensure we can match correctly. +normalizeTypeParamsRegex = re.compile(r"type-parameter-\d+-\d+") +def normalizeTypeParams( line ): + return normalizeTypeParamsRegex.sub("type-parameter-?-?", line) + +# reading as binary (since we known it is pure ascii) is much faster than reading as unicode +with io.open("loplugin.inlinefields.log", "rb", buffering=1024*1024) as txt: + for line in txt: + tokens = line.strip().split("\t") + if tokens[0] == "definition:": + parentClass = normalizeTypeParams(tokens[1]) + fieldName = normalizeTypeParams(tokens[2]) + sourceLocation = tokens[3] + fieldInfo = (parentClass, fieldName) + definitionSet.add(fieldInfo) + definitionToSourceLocationMap[fieldInfo] = sourceLocation + elif tokens[0] == "excluded:": + parentClass = normalizeTypeParams(tokens[1]) + fieldName = normalizeTypeParams(tokens[2]) + fieldInfo = (parentClass, fieldName) + excludedSet.add(fieldInfo) + elif tokens[0] == "deletedInDestructor:": + parentClass = normalizeTypeParams(tokens[1]) + fieldName = normalizeTypeParams(tokens[2]) + fieldInfo = (parentClass, fieldName) + deletedInDestructorSet.add(fieldInfo) + elif tokens[0] == "newedInConstructor:": + parentClass = normalizeTypeParams(tokens[1]) + fieldName = normalizeTypeParams(tokens[2]) + fieldInfo = (parentClass, fieldName) + newedInConstructorSet.add(fieldInfo) + else: + print( "unknown line: " + line) + +# Invert the definitionToSourceLocationMap +# If we see more than one method at the same sourceLocation, it's being autogenerated as part of a template +# and we should just ignore it +sourceLocationToDefinitionMap = {} +for k, v in definitionToSourceLocationMap.iteritems(): + sourceLocationToDefinitionMap[v] = sourceLocationToDefinitionMap.get(v, []) + sourceLocationToDefinitionMap[v].append(k) +for k, definitions in sourceLocationToDefinitionMap.iteritems(): + if len(definitions) > 1: + for d in definitions: + definitionSet.remove(d) + +tmp1list = list() +for d in definitionSet: +# TODO see comment in InlineFields::VisitCXXDeleteExpr +# if d in excludedSet or d not in deletedInDestructorSet or d not in newedInConstructorSet: + if d in excludedSet or d not in newedInConstructorSet: + continue + srcLoc = definitionToSourceLocationMap[d]; + tmp1list.append((d[0] + " " + d[1], srcLoc)) + +# sort results by filename:lineno +def natural_sort_key(s, _nsre=re.compile('([0-9]+)')): + return [int(text) if text.isdigit() else text.lower() + for text in re.split(_nsre, s)] +tmp1list.sort(key=lambda v: natural_sort_key(v[1])) + +# print out the results +with open("loplugin.inlinefields.report", "wt") as f: + for v in tmp1list: + f.write(v[1] + "\n") + f.write(" " + v[0] + "\n") + + -- cgit