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

Fix up edge cases in binsearch unit tests #240

Merged
merged 1 commit into from May 3, 2018

Conversation

Projects
None yet
2 participants
@corwinjoy
Contributor

corwinjoy commented May 2, 2018

Got rid of the NULL cases in runit.binsearch.R.
Added explicit test in binsearch function to handle NA and missing key consistently (always return NA).
Fixed compiler warning in binsearch.c
Added ability to run just one unit test from doRUnit.R (you may want to leave this out).

@joshuaulrich

This comment has been minimized.

Show comment
Hide comment
@joshuaulrich

joshuaulrich May 2, 2018

Owner

What were the compiler warnings you give as the reason for removing the static keyword? And what compiler command throws the warning?

Owner

joshuaulrich commented May 2, 2018

What were the compiler warnings you give as the reason for removing the static keyword? And what compiler command throws the warning?

@corwinjoy

This comment has been minimized.

Show comment
Hide comment
@corwinjoy

corwinjoy May 2, 2018

Contributor
Contributor

corwinjoy commented May 2, 2018

@joshuaulrich

This comment has been minimized.

Show comment
Hide comment
@joshuaulrich

joshuaulrich May 2, 2018

Owner

Sorry I didn't say "thank you" for your feedback and changes before diving into questions. I really appreciate them!

Hmm, my understanding was that using static in the declaration would limit scope to the translation unit (i.e. binsearch.c). I don't see any warning even with -Wall -Wextra. I'm using gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609. I'll try upgrading to see if that makes a difference.

Here the R CMD INSTALL output showing the entire command line for the gcc calls around binsearch.c. Perhaps you were using a different set of options, and one of them caused the warning?

> R CMD INSTALL xts_0.10-2.1.tar.gz
* installing to library ‘/home/josh/R/library’
* installing *source* package ‘xts’ ...                                                                                                                                                                                                                                                                                     ** libs
gcc -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/josh/R/library/zoo/include"    -fpic  -O3 -g0 -Wall -Wextra -pipe -pedantic -std=gnu99 -c add_class.c -o add_class.o
gcc -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/josh/R/library/zoo/include"    -fpic  -O3 -g0 -Wall -Wextra -pipe -pedantic -std=gnu99 -c any.c -o any.o
gcc -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/josh/R/library/zoo/include"    -fpic  -O3 -g0 -Wall -Wextra -pipe -pedantic -std=gnu99 -c attr.c -o attr.o
attr.c: In function ‘add_xtsCoreAttributes’:
attr.c:177:55: warning: unused parameter ‘_indexClass’ [-Wunused-parameter]
 SEXP add_xtsCoreAttributes(SEXP _x, SEXP _index, SEXP _indexClass, SEXP _tzone,
                                                       ^
gcc -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/josh/R/library/zoo/include"    -fpic  -O3 -g0 -Wall -Wextra -pipe -pedantic -std=gnu99 -c binsearch.c -o binsearch.o
gcc -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/josh/R/library/zoo/include"    -fpic  -O3 -g0 -Wall -Wextra -pipe -pedantic -std=gnu99 -c coredata.c -o coredata.o
Owner

joshuaulrich commented May 2, 2018

Sorry I didn't say "thank you" for your feedback and changes before diving into questions. I really appreciate them!

Hmm, my understanding was that using static in the declaration would limit scope to the translation unit (i.e. binsearch.c). I don't see any warning even with -Wall -Wextra. I'm using gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609. I'll try upgrading to see if that makes a difference.

Here the R CMD INSTALL output showing the entire command line for the gcc calls around binsearch.c. Perhaps you were using a different set of options, and one of them caused the warning?

> R CMD INSTALL xts_0.10-2.1.tar.gz
* installing to library ‘/home/josh/R/library’
* installing *source* package ‘xts’ ...                                                                                                                                                                                                                                                                                     ** libs
gcc -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/josh/R/library/zoo/include"    -fpic  -O3 -g0 -Wall -Wextra -pipe -pedantic -std=gnu99 -c add_class.c -o add_class.o
gcc -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/josh/R/library/zoo/include"    -fpic  -O3 -g0 -Wall -Wextra -pipe -pedantic -std=gnu99 -c any.c -o any.o
gcc -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/josh/R/library/zoo/include"    -fpic  -O3 -g0 -Wall -Wextra -pipe -pedantic -std=gnu99 -c attr.c -o attr.o
attr.c: In function ‘add_xtsCoreAttributes’:
attr.c:177:55: warning: unused parameter ‘_indexClass’ [-Wunused-parameter]
 SEXP add_xtsCoreAttributes(SEXP _x, SEXP _index, SEXP _indexClass, SEXP _tzone,
                                                       ^
gcc -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/josh/R/library/zoo/include"    -fpic  -O3 -g0 -Wall -Wextra -pipe -pedantic -std=gnu99 -c binsearch.c -o binsearch.o
gcc -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/josh/R/library/zoo/include"    -fpic  -O3 -g0 -Wall -Wextra -pipe -pedantic -std=gnu99 -c coredata.c -o coredata.o
@corwinjoy

This comment has been minimized.

Show comment
Hide comment
@corwinjoy

corwinjoy May 2, 2018

Contributor
Contributor

corwinjoy commented May 2, 2018

Fix up edge cases in binsearch unit tests
Remove the start = NULL test cases, since it will no longer be
supported. Always return NA if key is NA or zero-length. Fix compiler
warning in binsearch.c ('static' is unnecessary as the struct is
already local to the file).

See #100.

@joshuaulrich joshuaulrich merged commit c1e8d85 into joshuaulrich:window-xts May 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@joshuaulrich

This comment has been minimized.

Show comment
Hide comment
@joshuaulrich

joshuaulrich May 3, 2018

Owner

Thanks for the explanation and links regarding static on type declarations!

I did revert the change in doRunit.R. I'm working on a Makefile that will allow you to run single test files, which I need to commit...

I also moved the key length and NA checks to C. I generally prefer putting checks for C functions in C because it's possible to call them directly from C, which would miss any checks done only in R. The checks can sometimes be faster in C also.

Owner

joshuaulrich commented May 3, 2018

Thanks for the explanation and links regarding static on type declarations!

I did revert the change in doRunit.R. I'm working on a Makefile that will allow you to run single test files, which I need to commit...

I also moved the key length and NA checks to C. I generally prefer putting checks for C functions in C because it's possible to call them directly from C, which would miss any checks done only in R. The checks can sometimes be faster in C also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment