-
Notifications
You must be signed in to change notification settings - Fork 291
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
Remove dependency on strings.h #481
Conversation
213d5f2
to
e301081
Compare
Looks like the
@iphydf How should we deal with it, is disabling |
Or we can write own |
e301081
to
62704f0
Compare
Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed. testing/misc_tools.c, line 53 at r1 (raw file):
add a comment that this is implemented because strings.h is not available on MSVC. Comments from Reviewable |
62704f0
to
16a6ce5
Compare
testing/misc_tools.c, line 53 at r1 (raw file): Previously, cebe (Carsten Brandt) wrote…
Added. Comments from Reviewable |
Reviewed 1 of 2 files at r1, 1 of 1 files at r2. Comments from Reviewable |
Reviewed 1 of 2 files at r1, 1 of 1 files at r2. Comments from Reviewable |
@cebe Why do you lgtm, if current build is failed? |
@Diadlo sorry I did not notice that, was looking at the code only. |
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
f364a36
to
27bb4b7
Compare
Reviewed 1 of 1 files at r3, 1 of 1 files at r4. CMakeLists.txt, line 99 at r4 (raw file):
Maybe put inside Comments from Reviewable |
CMakeLists.txt, line 99 at r4 (raw file): Previously, Diadlo (Polshakov Dmitry) wrote…
Do we really want this warning to be enabled in the first place? Comments from Reviewable |
could you write a test for this function? Reviewed 1 of 2 files at r1, 1 of 1 files at r3, 1 of 1 files at r4. Comments from Reviewable |
Uh, sure, if you really want. I thought the function was trivial enough to verify that it does the right thing. Hm, it's probably easier to just replace this function with an ifdef using |
e2720f1
to
b88a302
Compare
Added the test. Hopefully the 20 minutes I have spent on it were worth it. Also, I ran out of wafers while writing it, thus the wafer test cases. |
auto_tests/tox_strncasecmp_test.c
Outdated
verify("The Wafers Are Salty.", "The wafers are sweet.", 17, NEGATIVE); | ||
verify("The Wafers Are Salty.", "The wafers are sweet.", -1, NEGATIVE); | ||
|
||
// the comparison should stop at first mistmatch |
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.
s/mist/mis/
Thanks, looks good. Can you fix the typo and then rebase? I'll merge it after. |
b88a302
to
f4711bc
Compare
auto_tests/tox_strncasecmp_test.c, line 164 at r5 (raw file): Previously, iphydf wrote…
Done. Comments from Reviewable |
Did I merge something after your rebase? I can't merge this, yet. Can you rebase (again)? |
Due to clang's tolower() macro being recursive.
Rebased. |
strings.h
is not available on MSVC.This change is