Skip to content

avoid some redundant checks in TokenList::readfile()#324

Merged
danmar merged 1 commit intocppcheck-opensource:masterfrom
firewave:readfile
Oct 8, 2023
Merged

avoid some redundant checks in TokenList::readfile()#324
danmar merged 1 commit intocppcheck-opensource:masterfrom
firewave:readfile

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

@firewave firewave commented Sep 28, 2023

calling isspace() is unnecessary since we handle all non-ASCII characters and already have special handling of all remaining whitespaces according to https://en.cppreference.com/w/cpp/string/byte/isspace.

Runinng with -q -D__GNUC__ -Ilib lib/valueflow.cpp

Clang 16 276,826,655 -> 270,642,840
GCC 13 283,762,095-> 280,179,302

@firewave
Copy link
Copy Markdown
Collaborator Author

Comment thread simplecpp.cpp
}

if (std::isspace(ch)) {
if (ch <= ' ') {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

hmm I am not sure. The compiler knows the ascii codes. But if we assume the developer does not.. isn't it more explicit that '\t' '\n' '\r' are handled using std::isspace(ch) than ch <= ' '.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We basically separate the ASCII table is three parts with our code:

>= 0x80 - non-ASCII characters we bail out on

< ' ' - whitespaces and control characters which we treat as allowed and converted to ' '

... and the printable ASCII characters

So we only have either ' ' or a printable ASCII so the only character isspace() can match is ' '. So we get rid of the conversion step and simply exclude the lower part of the table.

Copy link
Copy Markdown
Collaborator

@danmar danmar Oct 8, 2023

Choose a reason for hiding this comment

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

but with these changes you assume that < ' ' will match \t and \r and \n ... technically it does. but if we pretend it didn't then your changes would break things.

on the other hand we did assume that < ' ' would match all other control characters already.

I don't have a strong negative opinion about this .. just wanted to point out that there might be a good reason to be explicit. It extends to the tickets you wrote.. technically there are some false negative but I am skeptic about Cppcheck warnings.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't have a strong negative opinion about this .. just wanted to point out that there might be a good reason to be explicit

I think the code is explicit enough since we are limiting us to the printable ASCII characters. If we weren't ìsspace() would have been necessary.

@danmar danmar merged commit edb7b86 into cppcheck-opensource:master Oct 8, 2023
@firewave firewave deleted the readfile branch October 8, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants