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

Brodes/wcharcharconversion false positives upstream5 #17611

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

bdrodes
Copy link
Contributor

@bdrodes bdrodes commented Sep 27, 2024

Addressing false positives with byte arrays. There was an existing check for byte arrays, but there were cases missed. Updated and cleaned up this logic and added test cases to expose the observed false positive.

Also addressed false positives due to types based on macro definitions, and then dynamic checks are used to determine if dangerous widening is performed at runtime. Test cases added to demonstrate the style of check.

@bdrodes bdrodes marked this pull request as ready for review September 27, 2024 16:43
@bdrodes bdrodes requested a review from a team as a code owner September 27, 2024 16:43
@bdrodes
Copy link
Contributor Author

bdrodes commented Sep 27, 2024

@jketema Let me know if these commits give you what you need. I did screw something up, and failed to have my first commit stipulate explicitly the false positives in the test case, they were just marked 'GOOD'. I'll try to mark them explicitly for future PRs though.

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

Some comments

@bdrodes
Copy link
Contributor Author

bdrodes commented Sep 30, 2024

I believe I got all the comments. Is the protocol that I resolve them?

@jketema
Copy link
Contributor

jketema commented Sep 30, 2024

I believe I got all the comments. Is the protocol that I resolve them?

There's no real protocol. I've marked them as resolved.

@jketema
Copy link
Contributor

jketema commented Sep 30, 2024

I think you need to auto-format the ql file

@bdrodes
Copy link
Contributor Author

bdrodes commented Sep 30, 2024

I believe I got all the comments. Is the protocol that I resolve them?

There's no real protocol. I've marked them as resolved.

Fixed.

@jketema
Copy link
Contributor

jketema commented Sep 30, 2024

Thanks. I'll kick-off a DCA experiment. If that looks good, we can merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants