Skip to content

Commit

Permalink
Fixed #4369 (false positive: Variable 'i' is assigned a value that is…
Browse files Browse the repository at this point in the history 
… never used)
  • Loading branch information
@zingsheim
zingsheim committed Dec 4, 2012
1 parent 590704a commit aebdb37
Show file tree
Hide file tree
Showing 3 changed files with 179 additions and 37 deletions.
136 changes: 100 additions & 36 deletions lib/checkunusedvar.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ class Variables {
public:
enum VariableType { standard, array, pointer, reference, pointerArray, referenceArray, pointerPointer, none };

typedef std::set<unsigned int> VariableSet;
typedef std::list<VariableSet> VariableListOfSets;

This comment has been minimized.

Sign in to view

Copy link
@danmar

danmar Dec 4, 2012

Owner

I am not a fan of such typedefs.

In my humble opinion it is more clear in the code if you write

std::list<VariableSet>

Than if you write

VariableListOfSets

With the former code somebody can see immediately what it is and what can be done with it. With the latter code somebody has to lookup the typedef to see what it is.

Please remove these typedefs and write the type in the code instead.

This comment has been minimized.

Sign in to view

Copy link
@zingsheim

zingsheim Dec 5, 2012

Author Collaborator

Done. See b99d3f0.

/** Store information about variable usage */
class VariableUsage {
public:
Expand All @@ -54,7 +57,8 @@ class Variables {
}

/** variable is used.. set both read+write */
void use() {
void use(VariableListOfSets & varReadInScope) {
varReadInScope.back().insert(_var->varId());
_read = true;
_write = true;
}
Expand All @@ -78,6 +82,27 @@ class Variables {

typedef std::map<unsigned int, VariableUsage> VariableMap;

class ScopeGuard
{
public:
ScopeGuard(Variables & guarded,
bool insideLoop)
:_guarded(guarded),
_insideLoop(insideLoop)
{
_guarded.enterScope();
}

~ScopeGuard()
{
_guarded.leaveScope(_insideLoop);
}

private:
Variables & _guarded;
bool _insideLoop;
};

void clear() {
_varUsage.clear();
}
Expand All @@ -103,8 +128,17 @@ class Variables {
void eraseAll(unsigned int varid);
void clearAliases(unsigned int varid);

ScopeGuard newScope(bool insideLoop) {
return ScopeGuard(*this, insideLoop);
}

private:
void enterScope();
void leaveScope(bool insideLoop);

VariableMap _varUsage;
VariableListOfSets _varAddedInScope;
VariableListOfSets _varReadInScope;
};


Expand All @@ -123,7 +157,7 @@ void Variables::alias(unsigned int varid1, unsigned int varid2, bool replace)
// alias to self
if (varid1 == varid2) {
if (var1)
var1->use();
var1->use(_varReadInScope);
return;
}

Expand All @@ -150,8 +184,10 @@ void Variables::alias(unsigned int varid1, unsigned int varid2, bool replace)
var2->_aliases.insert(varid1);
var1->_aliases.insert(varid2);

if (var2->_type == Variables::pointer)
if (var2->_type == Variables::pointer) {
_varReadInScope.back().insert(varid2);
var2->_read = true;
}
}

void Variables::clearAliases(unsigned int varid)
Expand Down Expand Up @@ -196,8 +232,10 @@ void Variables::addVar(const Variable *var,
VariableType type,
bool write_)
{
if (var->varId() > 0)
if (var->varId() > 0) {
_varAddedInScope.back().insert(var->varId());
_varUsage.insert(std::make_pair(var->varId(), VariableUsage(var, type, false, write_, false)));
}
}

void Variables::allocateMemory(unsigned int varid, const Token* tok)
Expand All @@ -215,8 +253,10 @@ void Variables::read(unsigned int varid, const Token* tok)
VariableUsage *usage = find(varid);

if (usage) {
_varReadInScope.back().insert(varid);
usage->_read = true;
usage->_lastAccess = tok;
if (tok)
usage->_lastAccess = tok;
}
}

Expand All @@ -231,6 +271,7 @@ void Variables::readAliases(unsigned int varid, const Token* tok)
VariableUsage *aliased = find(*aliases);

if (aliased) {
_varReadInScope.back().insert(*aliases);
aliased->_read = true;
aliased->_lastAccess = tok;
}
Expand Down Expand Up @@ -285,7 +326,7 @@ void Variables::use(unsigned int varid, const Token* tok)
VariableUsage *usage = find(varid);

if (usage) {
usage->use();
usage->use(_varReadInScope);
usage->_lastAccess = tok;

std::set<unsigned int>::iterator aliases;
Expand All @@ -294,7 +335,7 @@ void Variables::use(unsigned int varid, const Token* tok)
VariableUsage *aliased = find(*aliases);

if (aliased) {
aliased->use();
aliased->use(_varReadInScope);
aliased->_lastAccess = tok;
}
}
Expand Down Expand Up @@ -332,6 +373,47 @@ Variables::VariableUsage *Variables::find(unsigned int varid)
return 0;
}

void Variables::enterScope()
{
_varAddedInScope.push_back(VariableSet());
_varReadInScope.push_back(VariableSet());
}

void Variables::leaveScope(bool insideLoop)
{
if (insideLoop) {
// read variables are read again in subsequent run through loop
VariableSet const & currentVarReadInScope = _varReadInScope.back();
for (VariableSet::const_iterator readIter = currentVarReadInScope.begin();
readIter != currentVarReadInScope.end();
++readIter)
{
read(*readIter, NULL);
}
}

VariableListOfSets::reverse_iterator reverseReadIter = _varReadInScope.rbegin();
++reverseReadIter;
if (reverseReadIter != _varReadInScope.rend())
{
// Transfer read variables into previous scope

VariableSet const & currentVarAddedInScope = _varAddedInScope.back();
VariableSet & currentVarReadInScope = _varReadInScope.back();
for (VariableSet::const_iterator addedIter = currentVarAddedInScope.begin();
addedIter != currentVarAddedInScope.end();
++addedIter)
{
currentVarReadInScope.erase(*addedIter);
}
VariableSet & previousVarReadInScope = *reverseReadIter;
previousVarReadInScope.insert(currentVarReadInScope.begin(),
currentVarReadInScope.end());
}
_varReadInScope.pop_back();
_varAddedInScope.pop_back();
}

static const Token* doAssignment(Variables &variables, const Token *tok, bool dereference, const Scope *scope)
{
// a = a + b;
Expand Down Expand Up @@ -589,8 +671,10 @@ static const Token * skipBracketsAndMembers(const Token *tok)
//---------------------------------------------------------------------------
// Usage of function variables
//---------------------------------------------------------------------------
void CheckUnusedVar::checkFunctionVariableUsage_iterateScopes(const Scope* const scope, Variables& variables, bool insideLoop, std::vector<unsigned int> &usedVariables)
void CheckUnusedVar::checkFunctionVariableUsage_iterateScopes(const Scope* const scope, Variables& variables, bool insideLoop)
{
Variables::ScopeGuard scopeGuard=variables.newScope(insideLoop);

// Find declarations if the scope is executable..
if (scope->isExecutable()) {
// Find declarations
Expand Down Expand Up @@ -648,12 +732,7 @@ void CheckUnusedVar::checkFunctionVariableUsage_iterateScopes(const Scope* const
if (tok->str() == "for" || tok->str() == "while" || tok->str() == "do") {
for (std::list<Scope*>::const_iterator i = scope->nestedList.begin(); i != scope->nestedList.end(); ++i) {
if ((*i)->classDef == tok) { // Find associated scope
checkFunctionVariableUsage_iterateScopes(*i, variables, true, usedVariables); // Scan child scope
insideLoop = false;
std::vector<unsigned int>::iterator it;
for (it = usedVariables.begin(); it != usedVariables.end(); ++it) {
variables.read((*it), tok);
}
checkFunctionVariableUsage_iterateScopes(*i, variables, true); // Scan child scope
tok = (*i)->classStart->link();
break;
}
Expand All @@ -664,7 +743,7 @@ void CheckUnusedVar::checkFunctionVariableUsage_iterateScopes(const Scope* const
if (tok->str() == "{") {
for (std::list<Scope*>::const_iterator i = scope->nestedList.begin(); i != scope->nestedList.end(); ++i) {
if ((*i)->classStart == tok) { // Find associated scope
checkFunctionVariableUsage_iterateScopes(*i, variables, insideLoop, usedVariables); // Scan child scope
checkFunctionVariableUsage_iterateScopes(*i, variables, false); // Scan child scope
tok = tok->link();
break;
}
Expand Down Expand Up @@ -827,10 +906,12 @@ void CheckUnusedVar::checkFunctionVariableUsage_iterateScopes(const Scope* const

// checked for chained assignments
if (tok != start && equal && equal->str() == "=") {
Variables::VariableUsage *var = variables.find(tok->varId());
unsigned int varId = tok->varId();
Variables::VariableUsage *var = variables.find(varId);

if (var && var->_type != Variables::reference)
var->_read = true;
if (var && var->_type != Variables::reference) {
variables.read(varId,tok);
}

tok = tok->previous();
}
Expand Down Expand Up @@ -868,78 +949,63 @@ void CheckUnusedVar::checkFunctionVariableUsage_iterateScopes(const Scope* const
variables.read(tok->next()->varId(), tok);
} else // addressof
variables.use(tok->next()->varId(), tok); // use = read + write
usedVariables.push_back(tok->next()->varId());
} else if (Token::Match(tok, ">> %var%")) {
variables.use(tok->next()->varId(), tok); // use = read + write
usedVariables.push_back(tok->next()->varId());
} else if (Token::Match(tok, "%var% >>|&") && Token::Match(tok->previous(), "[{};:]")) {
variables.read(tok->varId(), tok);
usedVariables.push_back(tok->next()->varId());
}

// function parameter
else if (Token::Match(tok, "[(,] %var% [")) {
variables.use(tok->next()->varId(), tok); // use = read + write
usedVariables.push_back(tok->next()->varId());
} else if (Token::Match(tok, "[(,] %var% [,)]") && tok->previous()->str() != "*") {
variables.use(tok->next()->varId(), tok); // use = read + write
usedVariables.push_back(tok->next()->varId());
} else if (Token::Match(tok, "[(,] (") &&
Token::Match(tok->next()->link(), ") %var% [,)]")) {
variables.use(tok->next()->link()->next()->varId(), tok); // use = read + write
usedVariables.push_back(tok->next()->link()->next()->varId());
}

// function
else if (Token::Match(tok, "%var% (")) {
variables.read(tok->varId(), tok);
usedVariables.push_back(tok->varId());
if (Token::Match(tok->tokAt(2), "%var% =")) {
variables.read(tok->tokAt(2)->varId(), tok);
usedVariables.push_back(tok->tokAt(2)->varId());
}
}

else if (Token::Match(tok, "[{,] %var% [,}]")) {
variables.read(tok->next()->varId(), tok);
usedVariables.push_back(tok->next()->varId());
}

else if (Token::Match(tok, "%var% .")) {
variables.use(tok->varId(), tok); // use = read + write
usedVariables.push_back(tok->varId());
}

else if (tok->isExtendedOp() &&
Token::Match(tok->next(), "%var%") && !Token::Match(tok->next(), "true|false|new") && tok->strAt(2) != "=") {
variables.readAll(tok->next()->varId(), tok);
usedVariables.push_back(tok->next()->varId());
}

else if (Token::Match(tok, "%var%") && tok->next() && (tok->next()->str() == ")" || tok->next()->isExtendedOp())) {
variables.readAll(tok->varId(), tok);
usedVariables.push_back(tok->varId());
}

else if (Token::Match(tok, "%var% ;") && Token::Match(tok->previous(), "[;{}:]")) {
variables.readAll(tok->varId(), tok);
usedVariables.push_back(tok->varId());
}

else if (Token::Match(tok, "++|-- %var%")) {
if (!Token::Match(tok->previous(), "[;{}:]"))
variables.use(tok->next()->varId(), tok);
else
variables.modified(tok->next()->varId(), tok);
usedVariables.push_back(tok->next()->varId());
}

else if (Token::Match(tok, "%var% ++|--")) {
if (!Token::Match(tok->previous(), "[;{}:]"))
variables.use(tok->varId(), tok);
else
variables.modified(tok->varId(), tok);
usedVariables.push_back(tok->varId());
}

else if (tok->isAssignmentOp()) {
Expand All @@ -949,7 +1015,6 @@ void CheckUnusedVar::checkFunctionVariableUsage_iterateScopes(const Scope* const
variables.write(tok2->varId(), tok);
else
variables.read(tok2->varId(), tok);
usedVariables.push_back(tok2->varId());
}
}
}
Expand All @@ -972,8 +1037,7 @@ void CheckUnusedVar::checkFunctionVariableUsage()
// varId, usage {read, write, modified}
Variables variables;

std::vector<unsigned int> usedVariables;
checkFunctionVariableUsage_iterateScopes(&*scope, variables, false, usedVariables);
checkFunctionVariableUsage_iterateScopes(&*scope, variables, false);


// Check usage of all variables in the current scope..
Expand Down
2 changes: 1 addition & 1 deletion lib/checkunusedvar.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class CPPCHECKLIB CheckUnusedVar : public Check {
}

/** @brief %Check for unused function variables */
void checkFunctionVariableUsage_iterateScopes(const Scope* const scope, Variables& variables, bool insideLoop, std::vector<unsigned int> &usedVariables);
void checkFunctionVariableUsage_iterateScopes(const Scope* const scope, Variables& variables, bool insideLoop);
void checkVariableUsage(const Scope* const scope, const Token* start, Variables& variables);
void checkFunctionVariableUsage();

Expand Down
Loading

1 comment on commit aebdb37

@danmar
Copy link
Owner

@danmar danmar commented on aebdb37 Dec 4, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It mostly looks good. I'd just like those typedefs removed. Thanks.

Please sign in to comment.