From 2f7ccd102a6280750ce920e4c4eab66f9a01b9d3 Mon Sep 17 00:00:00 2001 From: Noel Grandin Date: Thu, 17 Nov 2016 08:39:50 +0200 Subject: [PATCH] extend unusedfields loplugin to find fields that can be private and apply the results in xmlscript Change-Id: Ib126f6e1576639abfd171e99d9561be9715ece2f --- compilerplugins/clang/unusedfields.cxx | 143 +++++++++--------- compilerplugins/clang/unusedfields.py | 97 ++++++------ xmlscript/source/xml_helper/xml_impctx.cxx | 3 +- xmlscript/source/xmldlg_imexp/imp_share.hxx | 14 +- .../source/xmlflat_imexp/xmlbas_import.hxx | 2 + xmlscript/source/xmllib_imexp/imp_share.hxx | 2 +- xmlscript/source/xmlmod_imexp/imp_share.hxx | 2 +- 7 files changed, 129 insertions(+), 134 deletions(-) diff --git a/compilerplugins/clang/unusedfields.cxx b/compilerplugins/clang/unusedfields.cxx index 6e4da40439e18..1bf71cdea13bf 100644 --- a/compilerplugins/clang/unusedfields.cxx +++ b/compilerplugins/clang/unusedfields.cxx @@ -47,6 +47,7 @@ struct MyFieldInfo std::string fieldName; std::string fieldType; std::string sourceLocation; + std::string access; }; bool operator < (const MyFieldInfo &lhs, const MyFieldInfo &rhs) { @@ -57,6 +58,7 @@ bool operator < (const MyFieldInfo &lhs, const MyFieldInfo &rhs) // try to limit the voluminous output a little static std::set touchedSet; +static std::set touchedFromOutsideSet; static std::set readFromSet; static std::set definitionSet; @@ -76,11 +78,13 @@ class UnusedFields: std::string output; for (const MyFieldInfo & s : touchedSet) output += "touch:\t" + s.parentClass + "\t" + s.fieldName + "\n"; + for (const MyFieldInfo & s : touchedFromOutsideSet) + output += "outside:\t" + s.parentClass + "\t" + s.fieldName + "\n"; for (const MyFieldInfo & s : readFromSet) output += "read:\t" + s.parentClass + "\t" + s.fieldName + "\n"; for (const MyFieldInfo & s : definitionSet) { - output += "definition:\t" + s.parentClass + "\t" + s.fieldName + "\t" + s.fieldType + "\t" + s.sourceLocation + "\n"; + output += "definition:\t" + s.access + "\t" + s.parentClass + "\t" + s.fieldName + "\t" + s.fieldType + "\t" + s.sourceLocation + "\n"; } ofstream myfile; myfile.open( SRCDIR "/loplugin.unusedfields.log", ios::app | ios::out); @@ -89,14 +93,14 @@ class UnusedFields: } bool shouldVisitTemplateInstantiations () const { return true; } + bool shouldVisitImplicitCode() const { return true; } - bool VisitCallExpr(CallExpr* ); bool VisitFieldDecl( const FieldDecl* ); bool VisitMemberExpr( const MemberExpr* ); bool VisitDeclRefExpr( const DeclRefExpr* ); private: MyFieldInfo niceName(const FieldDecl*); - std::string fullyQualifiedName(const FunctionDecl*); + void checkForTouchedFromOutside(const FieldDecl* fieldDecl, const Expr* memberExpr, const MyFieldInfo& fieldInfo); }; MyFieldInfo UnusedFields::niceName(const FieldDecl* fieldDecl) @@ -111,73 +115,27 @@ MyFieldInfo UnusedFields::niceName(const FieldDecl* fieldDecl) aInfo.sourceLocation = std::string(name.substr(strlen(SRCDIR)+1)) + ":" + std::to_string(compiler.getSourceManager().getSpellingLineNumber(expansionLoc)); normalizeDotDotInFilePath(aInfo.sourceLocation); - return aInfo; -} - -std::string UnusedFields::fullyQualifiedName(const FunctionDecl* functionDecl) -{ - std::string ret = compat::getReturnType(*functionDecl).getCanonicalType().getAsString(); - ret += " "; - if (isa(functionDecl)) { - const CXXRecordDecl* recordDecl = dyn_cast(functionDecl)->getParent(); - ret += recordDecl->getQualifiedNameAsString(); - ret += "::"; - } - ret += functionDecl->getNameAsString() + "("; - bool bFirst = true; - for (const ParmVarDecl *pParmVarDecl : compat::parameters(*functionDecl)) { - if (bFirst) - bFirst = false; - else - ret += ","; - ret += pParmVarDecl->getType().getCanonicalType().getAsString(); - } - ret += ")"; - if (isa(functionDecl) && dyn_cast(functionDecl)->isConst()) { - ret += " const"; - } - - return ret; -} - -// prevent recursive templates from blowing up the stack -static std::set traversedFunctionSet; - -bool UnusedFields::VisitCallExpr(CallExpr* expr) -{ - // Note that I don't ignore ANYTHING here, because I want to get calls to my code that result - // from template instantiation deep inside the STL and other external code - - FunctionDecl* calleeFunctionDecl = expr->getDirectCallee(); - if (calleeFunctionDecl == nullptr) { - Expr* callee = expr->getCallee()->IgnoreParenImpCasts(); - DeclRefExpr* dr = dyn_cast(callee); - if (dr) { - calleeFunctionDecl = dyn_cast(dr->getDecl()); - if (calleeFunctionDecl) - goto gotfunc; - } - return true; + switch (fieldDecl->getAccess()) + { + case AS_public: aInfo.access = "public"; break; + case AS_private: aInfo.access = "private"; break; + case AS_protected: aInfo.access = "protected"; break; + default: aInfo.access = "unknown"; break; } -gotfunc: - // if we see a call to a function, it may effectively create new code, - // if the function is templated. However, if we are inside a template function, - // calling another function on the same template, the same problem occurs. - // Rather than tracking all of that, just traverse anything we have not already traversed. - if (traversedFunctionSet.insert(fullyQualifiedName(calleeFunctionDecl)).second) - TraverseFunctionDecl(calleeFunctionDecl); - - return true; + return aInfo; } bool UnusedFields::VisitFieldDecl( const FieldDecl* fieldDecl ) { fieldDecl = fieldDecl->getCanonicalDecl(); - const FieldDecl* canonicalDecl = fieldDecl; - - if( ignoreLocation( fieldDecl )) + 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(); // unwrap array types @@ -200,7 +158,7 @@ bool UnusedFields::VisitFieldDecl( const FieldDecl* fieldDecl ) return true; } */ - definitionSet.insert(niceName(canonicalDecl)); + definitionSet.insert(niceName(fieldDecl)); return true; } @@ -211,20 +169,22 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr ) if (!fieldDecl) { return true; } + 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; + } MyFieldInfo fieldInfo = niceName(fieldDecl); - // ignore move/copy operator, it's self->self - const FunctionDecl* parentFunction = parentFunctionDecl(memberExpr); - const CXXMethodDecl* methodDecl = dyn_cast_or_null(parentFunction); - if (!methodDecl || !(methodDecl->isCopyAssignmentOperator() || methodDecl->isMoveAssignmentOperator())) { - touchedSet.insert(fieldInfo); - } + // for the touched-from-outside analysis - // for the write-only analysis + checkForTouchedFromOutside(fieldDecl, memberExpr, fieldInfo); - if (ignoreLocation(memberExpr)) - return true; + // for the write-only analysis const Stmt* child = memberExpr; const Stmt* parent = parentStmt(memberExpr); @@ -298,13 +258,48 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr ) bool UnusedFields::VisitDeclRefExpr( const DeclRefExpr* declRefExpr ) { const Decl* decl = declRefExpr->getDecl(); - if (!isa(decl)) { + const FieldDecl* fieldDecl = dyn_cast(decl); + if (!fieldDecl) { + return true; + } + fieldDecl = fieldDecl->getCanonicalDecl(); + if (ignoreLocation(fieldDecl)) { return true; } - touchedSet.insert(niceName(dyn_cast(decl))); + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocation()))) { + return true; + } + MyFieldInfo fieldInfo = niceName(fieldDecl); + touchedSet.insert(fieldInfo); + checkForTouchedFromOutside(fieldDecl, declRefExpr, fieldInfo); return true; } +void UnusedFields::checkForTouchedFromOutside(const FieldDecl* fieldDecl, const Expr* memberExpr, const MyFieldInfo& fieldInfo) { + const FunctionDecl* memberExprParentFunction = parentFunctionDecl(memberExpr); + const CXXMethodDecl* methodDecl = dyn_cast_or_null(memberExprParentFunction); + + // it's touched from somewhere outside a class + if (!methodDecl) { + touchedSet.insert(fieldInfo); + touchedFromOutsideSet.insert(fieldInfo); + return; + } + + auto constructorDecl = dyn_cast(methodDecl); + if (methodDecl->isCopyAssignmentOperator() || methodDecl->isMoveAssignmentOperator()) { + // ignore move/copy operator, it's self->self + } else if (constructorDecl && (constructorDecl->isCopyConstructor() || constructorDecl->isMoveConstructor())) { + // ignore move/copy constructor, it's self->self + } else { + touchedSet.insert(fieldInfo); + if (memberExprParentFunction->getParent() != fieldDecl->getParent()) { + touchedFromOutsideSet.insert(fieldInfo); + } + } +} + loplugin::Plugin::Registration< UnusedFields > X("unusedfields", false); } diff --git a/compilerplugins/clang/unusedfields.py b/compilerplugins/clang/unusedfields.py index 785b7a9a1881a..7bf910f62d805 100755 --- a/compilerplugins/clang/unusedfields.py +++ b/compilerplugins/clang/unusedfields.py @@ -5,11 +5,13 @@ import io definitionSet = set() +protectedAndPublicDefinitionSet = set() # set of tuple(type, name) definitionToSourceLocationMap = dict() definitionToTypeMap = dict() callSet = set() readFromSet = set() sourceLocationSet = set() +touchedFromOutsideSet = 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. @@ -17,30 +19,43 @@ def normalizeTypeParams( line ): return normalizeTypeParamsRegex.sub("type-parameter-?-?", line) +def parseFieldInfo( tokens ): + if len(tokens) == 3: + return (normalizeTypeParams(tokens[1]), tokens[2]) + else: + return (normalizeTypeParams(tokens[1]), "") + # The parsing here is designed to avoid grabbing stuff which is mixed in from gbuild. # I have not yet found a way of suppressing the gbuild output. with io.open("loplugin.unusedfields.log", "rb", buffering=1024*1024) as txt: for line in txt: tokens = line.strip().split("\t") if tokens[0] == "definition:": - fieldInfo = (normalizeTypeParams(tokens[1]), tokens[2]) + access = tokens[1] + fieldInfo = (normalizeTypeParams(tokens[2]), tokens[3]) + srcLoc = tokens[5] + # ignore external source code + if (srcLoc.startswith("external/")): + continue + # ignore build folder + if (srcLoc.startswith("workdir/")): + continue definitionSet.add(fieldInfo) - definitionToTypeMap[fieldInfo] = tokens[3] - definitionToSourceLocationMap[fieldInfo] = tokens[4] + definitionToTypeMap[fieldInfo] = tokens[4] + if access == "protected" or access == "public": + protectedAndPublicDefinitionSet.add(fieldInfo) + definitionToSourceLocationMap[fieldInfo] = tokens[5] elif tokens[0] == "touch:": - if len(tokens) == 3: - callInfo = (normalizeTypeParams(tokens[1]), tokens[2]) - callSet.add(callInfo) - else: - callInfo = (normalizeTypeParams(tokens[1]), "") - callSet.add(callInfo) + callSet.add(parseFieldInfo(tokens)) + elif tokens[0] == "read:": + readFromSet.add(parseFieldInfo(tokens)) elif tokens[0] == "read:": - if len(tokens) == 3: - readInfo = (normalizeTypeParams(tokens[1]), tokens[2]) - readFromSet.add(readInfo) - else: - readInfo = (normalizeTypeParams(tokens[1]), "") - readFromSet.add(readInfo) + readFromSet.add(parseFieldInfo(tokens)) + elif tokens[0] == "outside:": + touchedFromOutsideSet.add(parseFieldInfo(tokens)) + 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 @@ -58,24 +73,6 @@ def normalizeTypeParams( line ): if d in callSet: continue srcLoc = definitionToSourceLocationMap[d]; - # ignore external source code - if (srcLoc.startswith("external/")): - continue - # ignore build folder - if (srcLoc.startswith("workdir/")): - continue - # ignore our stable/URE/UNO api - if (srcLoc.startswith("include/com/") - or srcLoc.startswith("include/cppu/") - or srcLoc.startswith("include/cppuhelper/") - or srcLoc.startswith("include/osl/") - or srcLoc.startswith("include/rtl/") - or srcLoc.startswith("include/sal/") - or srcLoc.startswith("include/salhelper/") - or srcLoc.startswith("include/systools/") - or srcLoc.startswith("include/typelib/") - or srcLoc.startswith("include/uno/")): - continue # this is all representations of on-disk data structures if (srcLoc.startswith("sc/source/filter/inc/scflt.hxx") or srcLoc.startswith("sw/source/filter/ww8/") @@ -108,24 +105,6 @@ def normalizeTypeParams( line ): if d in readFromSet: continue srcLoc = definitionToSourceLocationMap[d]; - # ignore external source code - if (srcLoc.startswith("external/")): - continue - # ignore build folder - if (srcLoc.startswith("workdir/")): - continue - # ignore our stable/URE/UNO api - if (srcLoc.startswith("include/com/") - or srcLoc.startswith("include/cppu/") - or srcLoc.startswith("include/cppuhelper/") - or srcLoc.startswith("include/osl/") - or srcLoc.startswith("include/rtl/") - or srcLoc.startswith("include/sal/") - or srcLoc.startswith("include/salhelper/") - or srcLoc.startswith("include/systools/") - or srcLoc.startswith("include/typelib/") - or srcLoc.startswith("include/uno/")): - continue # this is all representations of on-disk data structures if (srcLoc.startswith("sc/source/filter/inc/scflt.hxx") or srcLoc.startswith("sw/source/filter/ww8/") @@ -140,6 +119,17 @@ def normalizeTypeParams( line ): writeonlySet.add((clazz + " " + definitionToTypeMap[d], srcLoc)) + +canBePrivateSet = set() +for d in protectedAndPublicDefinitionSet: + clazz = d[0] + " " + d[1] + if d in touchedFromOutsideSet: + continue + srcLoc = definitionToSourceLocationMap[d]; + + canBePrivateSet.add((clazz + " " + definitionToTypeMap[d], srcLoc)) + + # sort the results using a "natural order" so sequences like [item1,item2,item10] sort nicely def natural_sort_key(s, _nsre=re.compile('([0-9]+)')): return [int(text) if text.isdigit() else text.lower() @@ -148,6 +138,7 @@ def natural_sort_key(s, _nsre=re.compile('([0-9]+)')): # sort results by name and line number tmp1list = sorted(untouchedSet, key=lambda v: natural_sort_key(v[1])) tmp2list = sorted(writeonlySet, key=lambda v: natural_sort_key(v[1])) +tmp3list = sorted(canBePrivateSet, key=lambda v: natural_sort_key(v[1])) # print out the results with open("loplugin.unusedfields.report-untouched", "wt") as f: @@ -158,5 +149,9 @@ def natural_sort_key(s, _nsre=re.compile('([0-9]+)')): for t in tmp2list: f.write( t[1] + "\n" ) f.write( " " + t[0] + "\n" ) +with open("loplugin.unusedfields.report-can-be-private", "wt") as f: + for t in tmp3list: + f.write( t[1] + "\n" ) + f.write( " " + t[0] + "\n" ) diff --git a/xmlscript/source/xml_helper/xml_impctx.cxx b/xmlscript/source/xml_helper/xml_impctx.cxx index c79e8d50a3e7a..52999fa83d9c8 100644 --- a/xmlscript/source/xml_helper/xml_impctx.cxx +++ b/xmlscript/source/xml_helper/xml_impctx.cxx @@ -79,9 +79,10 @@ struct ElementEntry class ExtendedAttributes; -struct MGuard +class MGuard { Mutex * m_pMutex; +public: explicit MGuard( Mutex * pMutex ) : m_pMutex( pMutex ) { if (m_pMutex) m_pMutex->acquire(); } diff --git a/xmlscript/source/xmldlg_imexp/imp_share.hxx b/xmlscript/source/xmldlg_imexp/imp_share.hxx index f185fcd0276eb..30c3f94408836 100644 --- a/xmlscript/source/xmldlg_imexp/imp_share.hxx +++ b/xmlscript/source/xmldlg_imexp/imp_share.hxx @@ -114,20 +114,20 @@ struct DialogImport : public ::cppu::WeakImplHelper< css::xml::input::XRoot > { friend class ImportContext; - +private: css::uno::Reference< css::uno::XComponentContext > _xContext; css::uno::Reference< css::util::XNumberFormatsSupplier > _xSupplier; std::shared_ptr< std::vector< OUString > > _pStyleNames; std::shared_ptr< std::vector< css::uno::Reference< css::xml::input::XElement > > > _pStyles; + css::uno::Reference< css::frame::XModel > _xDoc; +public: css::uno::Reference< css::container::XNameContainer > _xDialogModel; css::uno::Reference< css::lang::XMultiServiceFactory > _xDialogModelFactory; - css::uno::Reference< css::frame::XModel > _xDoc; sal_Int32 XMLNS_DIALOGS_UID, XMLNS_SCRIPT_UID; -public: inline bool isEventElement( sal_Int32 nUid, OUString const & rLocalName ) { @@ -156,8 +156,9 @@ public: : _xContext( xContext ) , _pStyleNames( pStyleNames ) , _pStyles( pStyles ) + , _xDoc( xDoc ) , _xDialogModel( xDialogModel ) - , _xDialogModelFactory( xDialogModel, css::uno::UNO_QUERY_THROW ), _xDoc( xDoc ) + , _xDialogModelFactory( xDialogModel, css::uno::UNO_QUERY_THROW ) , XMLNS_DIALOGS_UID( 0 ) , XMLNS_SCRIPT_UID( 0 ) { OSL_ASSERT( _xDialogModel.is() && _xDialogModelFactory.is() && @@ -168,9 +169,9 @@ public: , _xSupplier( rOther._xSupplier ) , _pStyleNames( rOther._pStyleNames ) , _pStyles( rOther._pStyles ) + , _xDoc( rOther._xDoc ) , _xDialogModel( rOther._xDialogModel ) , _xDialogModelFactory( rOther._xDialogModelFactory ) - , _xDoc( rOther._xDoc ) , XMLNS_DIALOGS_UID( rOther.XMLNS_DIALOGS_UID ) , XMLNS_SCRIPT_UID( rOther.XMLNS_SCRIPT_UID ) {} @@ -204,9 +205,10 @@ class ElementBase protected: DialogImport * const _pImport; ElementBase * const _pParent; - +private: const sal_Int32 _nUid; const OUString _aLocalName; +protected: const css::uno::Reference< css::xml::input::XAttributes > _xAttributes; public: diff --git a/xmlscript/source/xmlflat_imexp/xmlbas_import.hxx b/xmlscript/source/xmlflat_imexp/xmlbas_import.hxx index 5a913c6655332..194283c87d871 100644 --- a/xmlscript/source/xmlflat_imexp/xmlbas_import.hxx +++ b/xmlscript/source/xmlflat_imexp/xmlbas_import.hxx @@ -45,10 +45,12 @@ namespace xmlscript { protected: rtl::Reference m_xImport; + private: rtl::Reference m_xParent; OUString m_aLocalName; css::uno::Reference< css::xml::input::XAttributes > m_xAttributes; + protected: static bool getBoolAttr( bool* pRet, const OUString& rAttrName, const css::uno::Reference< css::xml::input::XAttributes >& xAttributes, sal_Int32 nUid ); diff --git a/xmlscript/source/xmllib_imexp/imp_share.hxx b/xmlscript/source/xmllib_imexp/imp_share.hxx index 74560ece78a6a..71d7d9435be44 100644 --- a/xmlscript/source/xmllib_imexp/imp_share.hxx +++ b/xmlscript/source/xmllib_imexp/imp_share.hxx @@ -158,7 +158,7 @@ class LibElementBase protected: rtl::Reference mxImport; rtl::Reference mxParent; - +private: OUString _aLocalName; css::uno::Reference< css::xml::input::XAttributes > _xAttributes; diff --git a/xmlscript/source/xmlmod_imexp/imp_share.hxx b/xmlscript/source/xmlmod_imexp/imp_share.hxx index b7ae02ceac02a..1f170a0dc8ca8 100644 --- a/xmlscript/source/xmlmod_imexp/imp_share.hxx +++ b/xmlscript/source/xmlmod_imexp/imp_share.hxx @@ -50,6 +50,7 @@ struct ModuleImport ModuleDescriptor& mrModuleDesc; sal_Int32 XMLNS_SCRIPT_UID; +private: sal_Int32 XMLNS_LIBRARY_UID; sal_Int32 XMLNS_XLINK_UID; @@ -85,7 +86,6 @@ public: class ModuleElement : public ::cppu::WeakImplHelper< css::xml::input::XElement > { -protected: rtl::Reference mxImport; OUString _aLocalName;