-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
Refactoring suppressions code. #5550
Conversation
|
||
std::istringstream iss(comment.substr(2)); | ||
std::string word; | ||
iss >> word; | ||
if (word.substr(0, cppchecksuppress.size()) != cppchecksuppress) | ||
if (!cppchecksuppress.count(word)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); }
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
As these are not just refactorings but functional changes please add unit tests to make sure the new behavior is actually expected. |
530aa62
to
1c590b9
Compare
sure. thanks for your review! |
No description provided.