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

Replace most strlen with strnlen_s #747

Merged
merged 1 commit into from
Apr 16, 2018
Merged

Replace most strlen with strnlen_s #747

merged 1 commit into from
Apr 16, 2018

Conversation

yitam
Copy link
Contributor

@yitam yitam commented Apr 13, 2018

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 73.228% when pulling 93b9938 on yitam:strlen into c87e37f on Microsoft:dev.

@yitam yitam requested a review from david-puglielli April 13, 2018 22:29
#define stricmp strcasecmp
#define strnicmp strncasecmp
#define strnlen_s(s) strnlen_s(s, INT_MAX)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two definitions for strnlen_s? Why not put this line in Stringfunctions.h? Then you should be able to remove the definition from StringFunctions.cpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is just a macro, mainly for WIN32, because strnlen_s() is builtin. Besides, StringFunctions.h is not included in Windows build

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I see.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw have you checked that this compiles on all platforms? I'm just a bit concerned because strnlen_s is part of the c11 standard, not c++11.

Copy link
Contributor Author

@yitam yitam Apr 16, 2018

Choose a reason for hiding this comment

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

yes, I've run customized in Build and Functional Test plans last Friday

@codecov-io
Copy link

codecov-io commented Apr 13, 2018

Codecov Report

Merging #747 into dev will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #747      +/-   ##
==========================================
+ Coverage   78.86%   78.91%   +0.05%     
==========================================
  Files          25       25              
  Lines        7149     7157       +8     
==========================================
+ Hits         5638     5648      +10     
+ Misses       1511     1509       -2
Impacted Files Coverage Δ
...php-7.1.16-src/ext/pdo_sqlsrv/shared/core_sqlsrv.h 80.14% <0%> (ø) ⬆️
.../php/x86/php-7.1.16-src/ext/pdo_sqlsrv/pdo_dbh.cpp 92.3% <0%> (+0.05%) ⬆️
...php-7.1.16-src/ext/pdo_sqlsrv/shared/core_stmt.cpp 65.06% <0%> (+0.07%) ⬆️
...x86/php-7.1.16-src/ext/sqlsrv/shared/core_stmt.cpp 83.8% <0%> (+0.09%) ⬆️
...php-7.1.16-src/ext/pdo_sqlsrv/shared/core_conn.cpp 86.97% <0%> (+0.09%) ⬆️
...x86/php-7.1.16-src/ext/sqlsrv/shared/core_conn.cpp 81.33% <0%> (+0.13%) ⬆️
...x86/php-7.1.16-src/ext/sqlsrv/shared/core_sqlsrv.h 83.65% <0%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c87e37f...93b9938. Read the comment docs.

@yitam yitam merged commit 7a531d9 into microsoft:dev Apr 16, 2018
@yitam yitam deleted the strlen branch April 16, 2018 18:47
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.

4 participants