Fix CheckClass::operatorEqToSelf by alonlib12 · Pull Request #3088 · cppcheck-opensource/cppcheck

alonlib12 · 2021-01-26T20:26:10Z

No description provided.

alonlib12 · 2021-01-27T17:28:29Z

Fix CheckClass::operatorEqToSelf false negative cases.

danmar · 2021-01-29T13:39:10Z

+	return res;
+}
+
+const Token * CheckClass::getScopeStartToken(const Token *tok)


hmm .. this is a strange name. With this name I would have thought it worked like tok->scope()->bodyStart.

I guess I'd prefer that you use tok->astTop() instead of a for loop. I assume that we only are interested in the top.

Then instead of such condition: itr->link()->next() && itr->link()->next()->str() == "{" I'd prefer Token::simpleMatch(itr->link(), ") {"). That is more explicit and also avoid extra calls for link() and next().

I think the name is strange but do not have a suggestion right now.. feel free to suggest a name that is more specific.

danmar · 2021-01-29T13:40:59Z

+		if (itr->str() == "!=" && (itr->astOperand1()->str() == "true" || itr->astOperand2()->str() == "true")) {
+			res = !res;
+		}
+		else if (itr->str() == "!=" && (itr->astOperand1()->str() != "false" && itr->astOperand2()->str() != "false")) {


In general I prefer that we use valueflow instead of hardcoded constants. A false token has the known integer value 0..

(... astOperand1()->hasKnownIntValue() && .. astOperand1()->getKnownIntValue() != 0 )

.. and a true token has the value 1 ..

danmar · 2021-01-29T13:50:03Z

 }

-bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs)
+bool CheckClass::isInverseAssignmentTest(const Token *tok)


hmm.. spontaneously I have the feeling this has some overlap for what we want to achieve with astIsVariableComparison. maybe I am wrong.

isInverseAssignmentTest checks if a self-assignment test is inverse.
For example, for if (this == &rhs) and if (!(this != &rhs)) the function will return true, and for
if (this != &rhs) it will return false.
Can I achieve this with astIsVariableComparison?

hmm.. you can implement this for now.

And then I can try to see if some refactoring / improvements can be made later.

I think a name that explains what the function do is better than a name that explains when it is used. The "Assignment" has nothing to do with the functionality of the function.

Something like isInverted or something like that will work for me..

Maybe we should also consider that in practice the result is not false/true but false/maybe/true.

if (foo && this == &rhs)

The address of rhs may or may not be this when this condition is true/false.

If the result is "maybe" it's better to bailout and don't write warnings if that can avoid false positives.

danmar · 2021-01-29T13:51:00Z

sorry I forgot this PR. If you don't get a review in 1-2 days.. feel free to ping me. you can write @danmar in a comment.

danmar

Thanks for working on this! I think it looks promising, but I have more nits.

danmar · 2021-01-31T10:32:37Z

    return false;
 }

+bool CheckClass::isTrueKeyword(const Token* tok)


I'd prefer that this was a local function. Not part of CheckClass. No need to declare it in the header.

danmar · 2021-01-31T10:34:09Z

 }

-const Token * CheckClass::getScopeStartToken(const Token *tok)
+const Token * CheckClass::getIfStmtScopeStart(const Token *tok)


this name is better. but I'd suggest getIfStmtBodyStart.

danmar · 2021-01-31T10:36:35Z

 }

-bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs)
+bool CheckClass::isInverseAssignmentTest(const Token *tok)


hmm.. you can implement this for now.

And then I can try to see if some refactoring / improvements can be made later.

danmar · 2021-01-31T10:58:34Z

 }

-bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs)
+bool CheckClass::isInverseAssignmentTest(const Token *tok)


I think a name that explains what the function do is better than a name that explains when it is used. The "Assignment" has nothing to do with the functionality of the function.

Something like isInverted or something like that will work for me..

Maybe we should also consider that in practice the result is not false/true but false/maybe/true.

if (foo && this == &rhs)

The address of rhs may or may not be this when this condition is true/false.

If the result is "maybe" it's better to bailout and don't write warnings if that can avoid false positives.

alonlib12 · 2021-02-02T11:38:10Z

@danmar ping

danmar · 2021-02-02T14:02:33Z

I would like to add your name in our AUTHORS. What name would you like that I add?

alonlib12 · 2021-02-02T15:10:28Z

Alon Liberman Thanks :)

…

On Tue, Feb 2, 2021 at 4:02 PM Daniel Marjamäki ***@***.***> wrote: I would like to add your name in our AUTHORS. What name would you like that I add? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3088 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKE5AEIS6DFQEZ7X5A5BVF3S5AAYVANCNFSM4WUB6HQA> .

…r union member is never used" for structs with __attribute__((packed)) or #pragma pack(push))

alonlib12 added 2 commits January 26, 2021 22:18

Fix CheckClass::operatorEqToSelf

5d369f5

Fix 'hasAllocation' function declaration

ef20a74

Fix tests asserts

f62afd8

danmar reviewed Jan 29, 2021

View reviewed changes

Fix code review rejections

cd78a0a

danmar reviewed Jan 31, 2021

View reviewed changes

Fix code review rejections.

7e42a71

danmar merged commit a22e476 into cppcheck-opensource:main Feb 2, 2021

uhziel pushed a commit to uhziel/cppcheck that referenced this pull request Feb 25, 2021

Fixed cppcheck-opensource#3088 (False positive: Dont report "struct o…

0583763

…r union member is never used" for structs with __attribute__((packed)) or #pragma pack(push))

Conversation

alonlib12 commented Jan 26, 2021

Uh oh!

alonlib12 commented Jan 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar Jan 29, 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!

Choose a reason for hiding this comment

Uh oh!

alonlib12 Jan 30, 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 Jan 29, 2021

Uh oh!

danmar left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alonlib12 commented Feb 2, 2021

Uh oh!

danmar commented Feb 2, 2021

Uh oh!

alonlib12 commented Feb 2, 2021 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danmar Jan 29, 2021 •

edited

Loading

alonlib12 Jan 30, 2021 •

edited

Loading