-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Replace banned APIs with safe ones #1056
Conversation
14a4c27
to
47eb4fc
Compare
@lolgear done |
I neither like 1 as a length of desired string nor 2 as a max length of string in |
What's wrong with these values? 1 as length is simply The first is just "normal" IMHO. The second is arbitrary, but only for this specific location, so macros or static const won't do any good either. That's what comments are for. |
Generated by 🚫 Danger |
Codecov Report
@@ Coverage Diff @@
## master #1056 +/- ##
==========================================
- Coverage 39.65% 38.87% -0.79%
==========================================
Files 27 27
Lines 4017 4018 +1
==========================================
- Hits 1593 1562 -31
- Misses 2424 2456 +32
Continue to review full report at Codecov.
|
@ffried I think about "pure" solution here.
// If I am correct about comparison here. Not Sure
if (name != '\0' && name == '/' && *(++name) == '\0') {} |
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.
Vanishing dust in C runtime - Nice blow!
@ffried |
@lolgear You're actually right. We could express these as |
@ffried |
@lolgear What about it? 🙂 |
// If I am correct about comparison here. Not sure
if (name != '\0' && name == '/' && *(++name) == '\0') {} |
@lolgear I think it would be correct. But from a readability perspective much worse than |
@ffried |
@lolgear Fine by me. Let's wait for the CI 🙂 |
LGTM👍 |
New Pull Request Checklist
I have read and understood the CONTRIBUTING guide
I have read the Documentation
I have searched for a similar pull request in the project and found none
I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)
I have added the required tests to prove the fix/feature I am adding
I have updated the documentation (if necessary)
I have run the tests and they pass
I have run the lint and it passes (
pod lib lint
)This merge request fixes / refers to the following issues: #1050
Pull Request Description
This replaces the banned APIs in the list mentioned in #1050 with their safe counterparts.