avoid some redundant checks in TokenList::readfile()
#324
Conversation
|
I filed/updated several tickets about issues I discovered because of this: |
| @@ -692,7 +690,7 @@ void simplecpp::TokenList::readfile(Stream &stream, const std::string &filename, | |||
| continue; | |||
| } | |||
|
|
|||
| if (std::isspace(ch)) { | |||
| if (ch <= ' ') { | |||
There was a problem hiding this comment.
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 <= ' '.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.cppClang 16
276,826,655->270,642,840GCC 13
283,762,095->280,179,302