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 #11713 FN constParameterPointer/Reference with unused parameter #6085

Merged
merged 11 commits into from
Mar 8, 2024

Conversation

chrchr-github
Copy link
Collaborator

No description provided.

@chrchr-github
Copy link
Collaborator Author

I wonder how much noise this will cause.
But we already warn for

int f(int& r) {
	int i = r;
#ifdef MACRO
	r = 0;
#endif
	return i;
}

@danmar
Copy link
Owner

danmar commented Mar 8, 2024

But we already warn for

I also classify that as a false positive. I would prefer that we fix such FPs... if there are ifdefs and stuff in the scope that could hide code then do not warn.

However I would suggest that the "possible FP" warnings are still reported in such cases with a separate ID (maybe enabled by --inconclusive).

lib/checkother.cpp Outdated Show resolved Hide resolved
@firewave
Copy link
Collaborator

firewave commented Mar 8, 2024

I also classify that as a false positive. I would prefer that we fix such FPs... if there are ifdefs and stuff in the scope that could hide code then do not warn.

I do not agree. IMO that is the point of analyzing multiple configurations.

int f() {
	int i; // unassigned
#ifdef MACRO
	i = 0;
#endif
	return i; // uninitialized
}

In that case we correctly detect the unassigned variable and the uninitialized one. Not reporting it in the case this PR addresses would be inconsistent and thus unexpected.

@chrchr-github chrchr-github merged commit 54f46d3 into danmar:main Mar 8, 2024
64 checks passed
@chrchr-github chrchr-github deleted the chr_Fix11713 branch March 8, 2024 14: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.

3 participants