Fix up edge cases in binsearch unit tests #240
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).