Skip to content

Remove dependency on strings.h#481

Merged
iphydf merged 3 commits into
TokTok:masterfrom
nurupo:remove-strings-h
Mar 2, 2017
Merged

Remove dependency on strings.h#481
iphydf merged 3 commits into
TokTok:masterfrom
nurupo:remove-strings-h

Conversation

@nurupo

@nurupo nurupo commented Feb 20, 2017

Copy link
Copy Markdown
Member

strings.h is not available on MSVC.


This change is Reviewable

@nurupo nurupo added this to the v0.1.7 milestone Feb 20, 2017
@nurupo

nurupo commented Feb 20, 2017

Copy link
Copy Markdown
Member Author

Looks like the -Werror makes the clang build on Travis fail because clang's ctype.h header triggers -Wdisabled-macro-expansion. gcc and mingw don't have such an issue.

/home/travis/build/TokTok/c-toxcore/testing/misc_tools.c:56:18: error: disabled expansion of recursive macro [-Werror,-Wdisabled-macro-expansion]
        int c1 = tolower(*(s1++));
                 ^
/usr/include/ctype.h:230:35: note: expanded from macro 'tolower'
#  define tolower(c)    __tobody (c, tolower, *__ctype_tolower_loc (), (c))
                                     ^
/usr/include/ctype.h:176:14: note: expanded from macro '__tobody'
            __res = f args;                                                   \
                    ^

@iphydf How should we deal with it, is disabling -Wdisabled-macro-expansion just for clang acceptable?

@Diadlo

Diadlo commented Feb 20, 2017

Copy link
Copy Markdown

Or we can write own tolower

@nurupo nurupo mentioned this pull request Feb 22, 2017
@cebe

cebe commented Feb 22, 2017

Copy link
Copy Markdown
Member

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):

}

int tox_strncasecmp(const char* s1, const char* s2, size_t n)

add a comment that this is implemented because strings.h is not available on MSVC.


Comments from Reviewable

@nurupo

nurupo commented Feb 22, 2017

Copy link
Copy Markdown
Member Author

testing/misc_tools.c, line 53 at r1 (raw file):

Previously, cebe (Carsten Brandt) wrote…

add a comment that this is implemented because strings.h is not available on MSVC.

Added.


Comments from Reviewable

@Diadlo

Diadlo commented Feb 22, 2017

Copy link
Copy Markdown

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@cebe

cebe commented Feb 22, 2017

Copy link
Copy Markdown
Member

:lgtm:


Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@Diadlo

Diadlo commented Feb 22, 2017

Copy link
Copy Markdown

@cebe Why do you lgtm, if current build is failed?

@cebe

cebe commented Feb 23, 2017

Copy link
Copy Markdown
Member

@Diadlo sorry I did not notice that, was looking at the code only.

@cebe

cebe commented Feb 23, 2017

Copy link
Copy Markdown
Member

:lgtm_cancel:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@Diadlo

Diadlo commented Feb 24, 2017

Copy link
Copy Markdown

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


CMakeLists.txt, line 99 at r4 (raw file):

    add_flag("-Wno-covered-switch-default")
    # Due to clang's tolower() macro being recursive https://github.com/TokTok/c-toxcore/pull/481
    add_flag("-Wno-disabled-macro-expansion")

Maybe put inside if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")?


Comments from Reviewable

@nurupo

nurupo commented Feb 24, 2017

Copy link
Copy Markdown
Member Author

CMakeLists.txt, line 99 at r4 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Maybe put inside if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")?

Do we really want this warning to be enabled in the first place?


Comments from Reviewable

@iphydf

iphydf commented Feb 26, 2017

Copy link
Copy Markdown
Member

:lgtm_strong: 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.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@nurupo

nurupo commented Feb 26, 2017

Copy link
Copy Markdown
Member Author

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 strncasecmp() on POSIX systems and _strnicmp() on Windows rather than writing tests for it.

@nurupo nurupo force-pushed the remove-strings-h branch 3 times, most recently from e2720f1 to b88a302 Compare February 26, 2017 18:16
@nurupo

nurupo commented Feb 26, 2017

Copy link
Copy Markdown
Member Author

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.

Comment thread auto_tests/tox_strncasecmp_test.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/mist/mis/

@iphydf

iphydf commented Feb 27, 2017

Copy link
Copy Markdown
Member

Thanks, looks good. Can you fix the typo and then rebase? I'll merge it after.

@nurupo

nurupo commented Feb 27, 2017

Copy link
Copy Markdown
Member Author

auto_tests/tox_strncasecmp_test.c, line 164 at r5 (raw file):

Previously, iphydf wrote…

s/mist/mis/

Done.


Comments from Reviewable

@iphydf

iphydf commented Mar 1, 2017

Copy link
Copy Markdown
Member

Did I merge something after your rebase? I can't merge this, yet. Can you rebase (again)?

@nurupo nurupo force-pushed the remove-strings-h branch from f4711bc to def92ab Compare March 2, 2017 01:44
@nurupo

nurupo commented Mar 2, 2017

Copy link
Copy Markdown
Member Author

Rebased.

@iphydf iphydf merged commit def92ab into TokTok:master Mar 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants