Skip to content
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

Fix #11311 Do not search for null pointer in dead code #6442

Merged
merged 5 commits into from
Jun 25, 2024

Conversation

francois-berder
Copy link
Contributor

No description provided.

@francois-berder
Copy link
Contributor Author

@chrchr-github
I fixed this typo.

Unfortunately, I could not find a way to skip tokens of the "false" branch when processing the ternary operator. I can see a function named skipTernaryOp but that does not seem to do what I want. Any suggestions how to skip tokens of a ternary operator branch safely?

@chrchr-github
Copy link
Collaborator

What kind of code do you have in mind?
There is no warning for e.g.

int f(const int* p) {
	if (p)
		return 0;
	return 0 ? *p : 1;
}

@pfultz2
Copy link
Contributor

pfultz2 commented May 27, 2024

findTokensSkipDeadCode is the function you want to use. It should skip the ternary operator as well.

@francois-berder francois-berder marked this pull request as draft May 31, 2024 13:53
@francois-berder
Copy link
Contributor Author

Converting this PR to draft.
I am trying to handle more scenarios such as else if branch, nested branches, etc.

@pfultz2
Copy link
Contributor

pfultz2 commented Jun 1, 2024

There should be no need to reinvent the wheel here. findTokensSkipDeadCode should work for nested branches, else if, early returns, lambda captures, and ternary operator, and it can be easily extended to handle unevaluated functions as well.

@francois-berder
Copy link
Contributor Author

There should be no need to reinvent the wheel here. findTokensSkipDeadCode should work for nested branches, else if, early returns, lambda captures, and ternary operator, and it can be easily extended to handle unevaluated functions as well.

You are right, using findTokensSkipDeadCode makes it much easier to fix this bug.

@francois-berder francois-berder marked this pull request as ready for review June 2, 2024 19:23
@francois-berder
Copy link
Contributor Author

@pfultz2 @chrchr-github I think this PR is now again ready for review.

return true;

while (tok) {
if (isUnevaluated(tok->previous()))
Copy link
Owner

Choose a reason for hiding this comment

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

could you clarify what will tok->previous() be. if there is parenthesis like ...)tok.. or something then you want to return true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to make sure that the following code does not trigger a warning.

void f(type* p) {
   x(sizeof ( (p[0])));
    if (!p)
        ;
}

I need to check whether the variable p is unevaluated so I used a similar method found in checkuninitvar.cpp (see method CheckUninitVar::checkExpr).

void CheckNullPointer::nullPointerByDeRefAndChec()
static bool isPointerUnevaluated(const Token *tok)
{
if (isUnevaluated(tok))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be checked in findTokensSkipDeadCodeImpl function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danmar @pfultz2 I updated my PR to move this in findTokensSkipDeadAndUnevaluatedCode function because I was not very happy with the way I detected unevaluated pointers.

@francois-berder
Copy link
Contributor Author

@danmar @pfultz2 @chrchr-github ping

@chrchr-github chrchr-github merged commit 1668b0b into danmar:main Jun 25, 2024
63 checks passed
@francois-berder francois-berder deleted the fix-11311 branch June 27, 2024 08:48
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.

4 participants