-
Notifications
You must be signed in to change notification settings - Fork 67
Package Contracts5 #113
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
Package Contracts5 #113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mbaluda ! I've made some initial comments, will need to come back for a more deep look at implementation after you've had some thoughts on these comments :)
you always have the most "case heavy" rules it seems!
c/cert/src/rules/ERR33-C/DetectAndHandleStandardLibraryErrors.ql
Outdated
Show resolved
Hide resolved
c/cert/src/rules/ERR33-C/DetectAndHandleStandardLibraryErrors.ql
Outdated
Show resolved
Hide resolved
c/cert/src/rules/ERR33-C/DetectAndHandleStandardLibraryErrors.ql
Outdated
Show resolved
Hide resolved
@knewbury01 I added the implementation for rule ERR32-C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERR32-C reviewed! I was trying to be quite picky on edge cases but I am not totally sure if my testcase suggestions are realistic. otherwise looking good, these rules are very cool !
will submit a separate (re)review for ERR33-C
c/cert/src/rules/ERR32-C/DoNotRelyOnIndeterminateValuesOfErrno.ql
Outdated
Show resolved
Hide resolved
Rule ERR33-C was modified to improve the performance and to avoid false negatives arising when an expected error check expression is not present in the DB (see the added test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbaluda both queries reviewed,
ERR33-C looks good to me !
2 minor questions on ERR32-C, but after those tiny things I think it will be good to go
c/cert/src/rules/ERR32-C/DoNotRelyOnIndeterminateValuesOfErrno.ql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Change request type
.ql
,.qll
,.qls
or unit tests)Rules with added or modified queries
Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
Query development review checklist
For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:
Author
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Reviewer
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.