Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactoring suppressions code. #5550

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Refactoring suppressions code. #5550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c590b9
Refactoring suppressions code.
danmar Oct 13, 2023
dd7c5a0
test
danmar Oct 13, 2023
137b8fb
suppress
danmar Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/preprocessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,9 +274,9 @@ static void addInlineSuppressions(const simplecpp::TokenList &tokens, const Sett
}
}

std::for_each(inlineSuppressionsBlockBegin.begin(), inlineSuppressionsBlockBegin.end(), [&](const Suppressions::Suppression & suppr) {
for (const Suppressions::Suppression & suppr: inlineSuppressionsBlockBegin)
// cppcheck-suppress useStlAlgorithm
bad.emplace_back(suppr.fileName, suppr.lineNumber, "Suppress Begin: No matching end");
});
}

void Preprocessor::inlineSuppressions(const simplecpp::TokenList &tokens, Suppressions &suppressions)
Expand Down
27 changes: 16 additions & 11 deletions lib/suppressions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ std::vector<Suppressions::Suppression> Suppressions::parseMultiSuppressComment(c
return suppressions;
}

const std::string SymbolNameString = "symbolName=";
const std::string symbolNameString = "symbolName=";

while (iss) {
std::string word;
Expand All @@ -174,8 +174,8 @@ std::vector<Suppressions::Suppression> Suppressions::parseMultiSuppressComment(c
break;
if (word.find_first_not_of("+-*/%#;") == std::string::npos)
break;
if (startsWith(word, SymbolNameString)) {
s.symbolName = word.substr(SymbolNameString.size());
if (startsWith(word, symbolNameString)) {
s.symbolName = word.substr(symbolNameString.size());
} else {
if (errorMessage && errorMessage->empty())
*errorMessage = "Bad multi suppression '" + comment + "'. legal format is cppcheck-suppress[errorId, errorId symbolName=arr, ...]";
Expand Down Expand Up @@ -304,28 +304,33 @@ bool Suppressions::Suppression::parseComment(std::string comment, std::string *e
if (comment.compare(comment.size() - 2, 2, "*/") == 0)
comment.erase(comment.size() - 2, 2);

const std::string cppchecksuppress = "cppcheck-suppress";
const std::set<std::string> cppchecksuppress{
"cppcheck-suppress",
"cppcheck-suppress-begin",
"cppcheck-suppress-end",
"cppcheck-suppress-file"
};

std::istringstream iss(comment.substr(2));
std::string word;
iss >> word;
if (word.substr(0, cppchecksuppress.size()) != cppchecksuppress)
if (!cppchecksuppress.count(word))
Copy link
Collaborator

Choose a reason for hiding this comment

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

That changes the behavior and will most likely impact the performance negatively. It should be using startsWith().

Also if you do not use the actual contents of the count() result you should use std::find() as that stops at the first occurrence instead of continuing to look for all of them.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Also if you do not use the actual contents of the count() result you should use std::find() as that stops at the first occurrence instead of continuing to look for all of them.

for a std::set, it is technically the same. here is the implementation for count on my computer (/usr/include/c++/11/bits/stl_set.h):

           size_type
           count(const key_type& __x) const
           { return _M_t.find(__x) == _M_t.end() ? 0 : 1; }

and the find() also invokes the same _M_t.find call:

      const_iterator
      find(const key_type& __x) const
      { return _M_t.find(__x); }

Copy link
Owner Author

Choose a reason for hiding this comment

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

That changes the behavior and will most likely impact the performance negatively. It should be using startsWith().

but the point is that I don't want to match random strings.. I want it to be explicit. It used to be explicit before JohanBertrand changed it..

Copy link
Collaborator

Choose a reason for hiding this comment

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

for a std::set, it is technically the same.

That is true. I did not check the container.

return false;

iss >> errorId;
if (!iss)
return false;

const std::string SymbolNameString = "symbolName=";
const std::string symbolNameString = "symbolName=";

while (iss) {
iss >> word;
if (!iss)
break;
if (word.find_first_not_of("+-*/%#;") == std::string::npos)
break;
if (startsWith(word, SymbolNameString))
symbolName = word.substr(SymbolNameString.size());
if (startsWith(word, symbolNameString))
symbolName = word.substr(symbolNameString.size());
else if (errorMessage && errorMessage->empty())
*errorMessage = "Bad suppression attribute '" + word + "'. You can write comments in the comment after a ; or //. Valid suppression attributes; symbolName=sym";
}
Expand Down Expand Up @@ -395,16 +400,16 @@ std::string Suppressions::Suppression::getText() const
bool Suppressions::isSuppressed(const Suppressions::ErrorMessage &errmsg, bool global)
{
const bool unmatchedSuppression(errmsg.errorId == "unmatchedSuppression");
bool return_value = false;
bool returnValue = false;
for (Suppression &s : mSuppressions) {
danmar marked this conversation as resolved.
Show resolved Hide resolved
if (!global && !s.isLocal())
continue;
if (unmatchedSuppression && s.errorId != errmsg.errorId)
continue;
if (s.isMatch(errmsg))
return_value = true;
returnValue = true;
}
return return_value;
return returnValue;
}

bool Suppressions::isSuppressed(const ::ErrorMessage &errmsg)
Expand Down
3 changes: 3 additions & 0 deletions test/testsuppressions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,6 +883,9 @@ class TestSuppressions : public TestFixture {
ASSERT_EQUALS(true, s.parseComment("/* cppcheck-suppress-end id */", &msg));
ASSERT_EQUALS("", msg);

// Bad cppcheck-suppress comment
ASSERT_EQUALS(false, s.parseComment("/* cppcheck-suppress-beggin id */", &msg));

// Bad attribute construction
const std::string badSuppressionAttribute = "Bad suppression attribute 'some'. You can write comments in the comment after a ; or //. Valid suppression attributes; symbolName=sym";

Expand Down
Loading