Skip to content

Fix #11842 FN constParameterPointer with library function#5257

Merged
chrchr-github merged 40 commits intocppcheck-opensource:mainfrom
chrchr-github:chr_Fix11842
Aug 5, 2023
Merged

Fix #11842 FN constParameterPointer with library function#5257
chrchr-github merged 40 commits intocppcheck-opensource:mainfrom
chrchr-github:chr_Fix11842

Conversation

@chrchr-github
Copy link
Copy Markdown
Collaborator

No description provided.

@chrchr-github
Copy link
Copy Markdown
Collaborator Author

chrchr-github commented Jul 21, 2023

I'm not sure if all of the new warnings should be suppressed, or if it's better to change the tests.

Changing the tests would reveal any false positives, and not distract from the original warnings the test was written for.

Comment thread lib/checkother.cpp
}
} else if (!parent->isCast()) {
const auto dir = mSettings->library.getArgDirection(ftok, argn + 1);
if (dir == Library::ArgumentChecks::Direction::DIR_IN)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems like this logic should be in isVariableChangedByFunctionCall, but I wonder why we skip it when the parent is a cast.

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github Jul 30, 2023

Choose a reason for hiding this comment

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

For this test:

void f(int, const int*);
void f(int i, int* p) {
    f(i, const_cast<const int*>(p));
}

Seems not ideal though.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

isVariableChangedByFunctionCall() already has handling for library functions, but it is not quite aligned with getTokenArgumentFunction() (constructors, init lists).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isVariableChangedByFunctionCall() already has handling for library functions, but it is not quite aligned with getTokenArgumentFunction() (constructors, init lists).

We should probably have an issue to track that.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If we only rely on isVariableChangedByFunctionCall(), there would be FPs for those:

struct A {
    int* x;
    A(int* y) : x(y) {}
};
void f(int* q) {
    int* a[1] = { q };
}

I'm not convinced that isVariableChangedByFunctionCall() should return true here.

@chrchr-github chrchr-github marked this pull request as ready for review August 3, 2023 09:50
@chrchr-github chrchr-github merged commit 7325154 into cppcheck-opensource:main Aug 5, 2023
@chrchr-github chrchr-github deleted the chr_Fix11842 branch August 5, 2023 16:48
mptre added a commit to mptre/cppcheck that referenced this pull request Aug 22, 2023
Commit 7325154 ("Fix #11842 FN constParameterPointer with library function
(cppcheck-opensource#5257)") most likely introduced a regression for (C) function pointers passed
to functions provided by the standard library that cppcheck has knowledge about.
mptre added a commit to mptre/cppcheck that referenced this pull request Aug 23, 2023
Commit 7325154 ("Fix #11842 FN constParameterPointer with library function
(cppcheck-opensource#5257)") most a regression for (C) function pointers passed to functions
provided by the standard library that cppcheck has knowledge about.
chrchr-github pushed a commit that referenced this pull request Aug 23, 2023
Commit 7325154 ("Fix #11842 FN constParameterPointer with library
function (#5257)") most likely introduced a regression for (C) function
pointers passed to functions provided by the standard library that
cppcheck has knowledge about.
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.

2 participants