Fix up edge cases in binsearch unit tests

[corwinjoy]

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]

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

[corwinjoy]

Basically, it was a warning that the storage specifier (static) does not apply for an object which is not instantiated. This makes sense, your struct here just defines a type, not any actual storage. So, it makes sense to get rid of the static specifier. This is with: gcc --version gcc (Ubuntu 6.3.0-12ubuntu2) 6.3.0 20170406 See, e.g. this discussion here https://stackoverflow.com/questions/12138742/the-member-variable-of-a-static-struct-in-c?rq=1 Basically, you are declaring a type but not any actual storage so static has no effect here.

…

On Tue, May 1, 2018 at 5:18 PM, Joshua Ulrich ***@***.***> wrote: What were the compiler warnings you give as the reason for removing the static keyword? And what compiler command throws the warning? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#240 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMJealTs1MMwfzhV55fdxXFbqmJCgXQBks5tuPs_gaJpZM4TurUr> .

[joshuaulrich]

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]

You're welcome, and I'm happy to give feed back. For the warning message, I guess I should stop being so lazy and give you more details :-). Here is the warning I see: sudo R CMD INSTALL xts_0.10-2.1.tar.gz * installing to library ‘/home/corwin/R/x86_64-pc-linux-gnu-library/3.4’ * installing *source* package ‘xts’ ... ** libs ... gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -I../inst/include -I"/home/corwin/R/x86_64-pc-linux-gnu-library/3.4/zoo/include" -fpic -g -O2 -fdebug-prefix-map=/bui ld/r-base-pIzt6B/r-base-3.4.3=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -c binsearch.c -o binsearch.o binsearch.c:14:1: warning: useless storage class specifier in empty declaration }; ^ Basically my understanding of the warning is this. The keyword static specifies that there shall be no external linkage for a definition outside the current translation unit (that is, the current file). This applies to variable and function definitions to make them local to a given file. But, what you have here is: static struct keyvec { double *dvec; double dkey; int *ivec; int ikey; }; In this case, your struct does not define an actual variable so there is nothing to link to outside of binsearch.c. Instead, you merely define a type here which is only visible to the code that follows the definition of this type. So, there is nothing for "static" to do here. Corwin P.S. See e.g. https://stackoverflow.com/questions/2743949/useless-class-storage-specifier-in-empty-declaration for another explanation of this.

…

On Wed, May 2, 2018 at 6:32 AM, Joshua Ulrich ***@***.***> wrote: 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 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#240 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMJeakMK-p5XmMqHelunPEDXuUWNEEMeks5tubWHgaJpZM4TurUr> .

[joshuaulrich]

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.