-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Conversation
What were the compiler warnings you give as the reason for removing the static keyword? And what compiler command throws the warning? |
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>
.
|
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 Here the
|
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>
.
|
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 joshuaulrich#100.
Thanks for the explanation and links regarding I did revert the change in I also moved the |
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).