Skip to content

Commit

Permalink
extend unusedfields loplugin to find fields that can be private
Browse files Browse the repository at this point in the history 
and apply the results in xmlscript

Change-Id: Ib126f6e1576639abfd171e99d9561be9715ece2f
  • Loading branch information
Noel Grandin committed Nov 17, 2016
1 parent 234325b commit 2f7ccd1
Show file tree
Hide file tree
Showing 7 changed files with 129 additions and 134 deletions.
143 changes: 69 additions & 74 deletions compilerplugins/clang/unusedfields.cxx
View file
Expand Up @@ -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)
{
Expand All @@ -57,6 +58,7 @@ bool operator < (const MyFieldInfo &lhs, const MyFieldInfo &rhs)

// try to limit the voluminous output a little
static std::set<MyFieldInfo> touchedSet;
static std::set<MyFieldInfo> touchedFromOutsideSet;
static std::set<MyFieldInfo> readFromSet;
static std::set<MyFieldInfo> definitionSet;

Expand All @@ -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);
Expand All @@ -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)
Expand All @@ -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<CXXMethodDecl>(functionDecl)) {
const CXXRecordDecl* recordDecl = dyn_cast<CXXMethodDecl>(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<CXXMethodDecl>(functionDecl) && dyn_cast<CXXMethodDecl>(functionDecl)->isConst()) {
ret += " const";
}

return ret;
}

// prevent recursive templates from blowing up the stack
static std::set<std::string> 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<DeclRefExpr>(callee);
if (dr) {
calleeFunctionDecl = dyn_cast<FunctionDecl>(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
Expand All @@ -200,7 +158,7 @@ bool UnusedFields::VisitFieldDecl( const FieldDecl* fieldDecl )
return true;
}
*/
definitionSet.insert(niceName(canonicalDecl));
definitionSet.insert(niceName(fieldDecl));
return true;
}

Expand All @@ -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<CXXMethodDecl>(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);
Expand Down Expand Up @@ -298,13 +258,48 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
bool UnusedFields::VisitDeclRefExpr( const DeclRefExpr* declRefExpr )
{
const Decl* decl = declRefExpr->getDecl();
if (!isa<FieldDecl>(decl)) {
const FieldDecl* fieldDecl = dyn_cast<FieldDecl>(decl);
if (!fieldDecl) {
return true;
}
fieldDecl = fieldDecl->getCanonicalDecl();
if (ignoreLocation(fieldDecl)) {
return true;
}
touchedSet.insert(niceName(dyn_cast<FieldDecl>(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<CXXMethodDecl>(memberExprParentFunction);

// it's touched from somewhere outside a class
if (!methodDecl) {
touchedSet.insert(fieldInfo);
touchedFromOutsideSet.insert(fieldInfo);
return;
}

auto constructorDecl = dyn_cast<CXXConstructorDecl>(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);

}
Expand Down
97 changes: 46 additions & 51 deletions compilerplugins/clang/unusedfields.py
View file
Expand Up @@ -5,42 +5,57 @@
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.
normalizeTypeParamsRegex = re.compile(r"type-parameter-\d+-\d+")
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
Expand All @@ -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/")
Expand Down Expand Up @@ -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/")
Expand All @@ -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()
Expand All @@ -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:
Expand All @@ -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" )


3 changes: 2 additions & 1 deletion xmlscript/source/xml_helper/xml_impctx.cxx
View file
Expand Up @@ -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(); }
Expand Down

0 comments on commit 2f7ccd1

Please sign in to comment.