Minor: const, shadow variables, bitwiseOnBoolean by chrchr-github · Pull Request #3450 · cppcheck-opensource/cppcheck

chrchr-github · 2021-09-07T14:14:48Z

No description provided.

danmar · 2021-09-07T20:38:30Z

            configset.insert(dtok->str());
    }
-    std::string cfg;
+    std::string c;


this spontanously feels unfortunate. c is not a good name. however it is only used in a very small scope so I could accept this. Is it possible to write a better name?

First I had cfgStr, which I thought was too long. Changed it back.

danmar · 2021-09-07T20:40:52Z

            error |= (*(p + 1) == '-');
        else if (*p == ':') {
-            error |= range | (*(p + 1) == '.');
+            error |= range || (*(p + 1) == '.');


this changes the sematics do we really want to have an extra condition here.. I am not sure we should activate the bitwiseOnBoolean in Cppcheck itself. I believe the main motivation is stylistic when people dislike to treat booleans as integers. And personally I don't have that problem with this.

Not sure what the change in semantics is in this case. Generally, binary | does not short-circuit, so if this is allowed, code will probably creep in which relies on side effects of the second condition.

maybe we should move bitwiseOnBoolean to an addon.

Isn't that just what severity style is intended for (which bitwiseOnBoolean has)? Addons don't seem very popular except the MISRA one...

the motivation for style checkers in core cppcheck should be to find bugs and I would say it should have a relatively high acceptance in the user community.

I am afraid that | is often used by intention often. As far as I know it is not widely considered bad practice / dangerous to use it.

Undo this change?

It seems like the check should be a performance check rather then a style check. The reason || is preferred is because it will short-circuit(and avoid extra unnecessary calculations). The only reason to use | with booleans is if it there is side effects and thus short-circuiting will not be equivalent.

I think for cppcheck's bitwiseOnBoolean it should check for side effects using isConstExpression, such checks like that cannot be done in an addon, so it needs to be done in cppcheck.

A more general check(ignoring side-effects) could be added to the cert.py addon as cert doens't have such an exception and in order to show it is intentional it requires extra set of parens.

Undo this change?

Yes please

The reason || is preferred is because it will short-circuit(and avoid extra unnecessary calculations)

I believe that the original motivation was that some people doesn't like to use bitwise on booleans for stylistic reasons. At that time we did not have addons. If that is the motivation then I feel that this should be moved to an addon however I guess I don't want to do that right now. Let's wait until after the release.

If we want to warn about a performance issue then I think the check should try to determine if the lhs condition is likely true. and if the rhs might be heavy. A purely semantic check of types is too simple.

I am no expert but I would assume that blindly replacing all | on boolean expressions with || can sometimes lead to slower code.

I believe that the original motivation was that some people doesn't like to use bitwise on booleans for stylistic reasons.

It does make the code much clearer as you can see the operations on booleans and have no side effects, whereas using | looks like you are not using boolean and the expressions may have side effects. This is the same reason why cppcheck tells users to make variables const with the constVariable check. Leaving the variable as non-const is still correct, just as using | is still correct, but the difference is that users can more clearly see the code doesn't have side effects.

And this extra clarity in code can also be used to find bugs. If the intention of the user is to have side effects and then cppcheck complains about constVariable or bitwiseOnBoolean then obviously something is wrong(and the solution in this case is not to add const or || but instead ensure the side effects are being properly called).

If that is the motivation then I feel that this should be moved to an addon

If we want a check that avoids FPs then it cannot be moved to an addon. A addon cannot call isConstExpression.

And sure the current check doesn't call isConstExpression, but it should to avoid FPs. So if we move this to an addon then we wont be able to fix those FPs. So this should definitely NOT be moved to an addon.

It does make the code much clearer as you can see the operations on booleans and have no side effects

I believe the original motivation was to always warn when booleans was used no matter if the expressions have side effects or not. It was by design to warn about something like b1|b2 where you just read two ordinary variables. I guess if you come from Java or something then that feels better. That rule can easily be moved to an addon.

If we can enhance this check so its focus will be to detect unsafe code; that is more interesting. Of course, that check does not have to be moved to an addon. However that would mean false negatives for those that still want to have the semantic warnings.

So I think we can easily implement the bitwiseOnBoolean in an addon as-is.. and it sounds ok to add some proper heuristics in the c++ code and rename the c++ check to unsafeBitwiseOnBoolean or something like that.

danmar · 2021-09-07T20:44:42Z


-        std::list<Suppressions::Suppression> inlineSuppressions;
-        if (!parseInlineSuppressionCommentToken(tok, inlineSuppressions, bad))
+        std::list<Suppressions::Suppression> inlineSuppr;


hmm the full name is better imho. Can we change the function name so it says something like addInlineSuppressions perhaps? The function name does not tell me what this function does.

Renamed the function, changed variable name back.

danmar · 2021-09-07T20:49:01Z

                                  const VariableMap &variableMap,
                                  const nonneg int scopeStartVarId,
-                                  std::map<int, std::map<std::string,int>>& structMembers);
+                                  std::map<int, std::map<std::string,int>>& structMembers) const;


I am not sure if this should be const. The name "set.." indicates it has some side effect? If it only modifies structMembers then I think it would be fine that it is const.

There are also setVarIdClassDeclaration and setVarIdClassFunction, which are const already.

hmm maybe the check is too pedantic.. this is why we write: "technically" the function can be const.

but it seems unfortunate to allow this noise.

it does not lead to bugs to have a non-const method as far as I know. but if you make it "const" and think it's safe to call it then there can be bugs.

I meant to write "setVarIdStructMembers and setVarIdClassFunction" above.
I think it's just good practice to make everything const that can be const, since it tells you something about what the function does. Although sometimes the compiler's idea of const is not aligned with the developer's (e.g. an object owns a pointer to a buffer, but the elements of the buffer can be modified without violating const rules)

As far as I see the tokenlist is modified by this method so it's not sensible to make this const.

So should const be removed from setVarIdStructMembers and setVarIdClassFunction also for consistency?

danmar · 2021-09-09T06:42:18Z


-        std::size_t processedsize = 0;
-        unsigned int c = 0;
+        std::size_t processedsize = 0, c = 0;


why std::size_t c .. it is not a data-size. it is a counter.

Because it is passed to a function that takes size_t?

I spotted it because MSVC warned about a potential overflow.

imho I do not feel that c should be a std::size_t .. I'd rather make it int and see if we can make MSVC happy. There will not be more than 2^32 files to check.

danmar · 2021-09-11T12:31:54Z

Sorry for slow reviews. Problem here is that I like some of this and feel we could discuss other changes. If you would put non-controversial changes in a separate PR that could be merged quickly.

danmar · 2021-09-12T19:27:05Z

            error |= has_dot | (!std::isdigit(*(p + 1)));
            has_dot = true;
-        } else if (*p == 'E' || *p == 'e') {
+        } else if ((*p == 'E') | (*p == 'e')) {


I think this code looks weird. I don't like "always use |" neither.

chrchr-github · 2021-09-13T09:23:53Z

Sorry for slow reviews. Problem here is that I like some of this and feel we could discuss other changes. If you would put non-controversial changes in a separate PR that could be merged quickly.

I have created a new PR here: #3456

…port-progress=<val>` to specify interval (#5353)

chrchr-github added 2 commits September 7, 2021 16:13

Minor: const, shadow variables, bitwiseOnBoolean

a6bd0c9

format

0179744

danmar reviewed Sep 7, 2021

View reviewed changes

chrchr-github added 4 commits September 8, 2021 11:20

Renamed function, better variable name

54b2ef0

Shadow variable, type consistency

36a0b5c

Undo const for functions that modify the token list

71bfe56

Consistently use operator |

db21afe

danmar reviewed Sep 9, 2021

View reviewed changes

Merge branch 'danmar:main' into main

efc5d66

danmar reviewed Sep 12, 2021

View reviewed changes

Merge branch 'danmar:main' into main

c04f2ef

chrchr-github marked this pull request as draft September 14, 2021 12:05

Merge branch 'danmar:main' into main

ede6fd1

chrchr-github closed this Sep 15, 2021

danmar pushed a commit that referenced this pull request Aug 23, 2023

refs #3450 (CLI --report-progress is currently useless) - added `--re…

0901e49

…port-progress=<val>` to specify interval (#5353)

Conversation

chrchr-github commented Sep 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar commented Sep 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrchr-github commented Sep 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danmar Sep 9, 2021 •

edited

Loading