Skip to content
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

gcc-12 always-false in fread.c #5476

Merged
merged 4 commits into from
Oct 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .dev/CRAN_Release.cmd
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ grep -En "for\s*[(]\s*[a-zA-Z0-9_]+\s*=" src/*.c | grep -Fv "#loop_counter_not_l

cd ..
R
cc(test=TRUE, clean=TRUE, CC="gcc-10") # to compile with -pedandic -Wall, latest gcc as CRAN: https://cran.r-project.org/web/checks/check_flavors.html
cc(test=TRUE, clean=TRUE, CC="gcc-12") # to compile with -pedandic -Wall, latest gcc as CRAN: https://cran.r-project.org/web/checks/check_flavors.html
saf = options()$stringsAsFactors
options(stringsAsFactors=!saf) # check tests (that might be run by user) are insensitive to option, #2718
test.data.table()
Expand Down Expand Up @@ -306,7 +306,7 @@ cd ~/build/R-devel-strict-clang
make

cd ~/build/R-devel-strict-gcc
# gcc-10 (in dev currently) failed to build R, so using regular gcc-9 (9.3.0 as per focal/Pop!_OS 20.04)
# gcc-10 failed to build R-devel at some point, so using regular gcc-9 (9.3.0 as per focal/Pop!_OS 20.04)
./configure --without-recommended-packages --disable-byte-compiled-packages --disable-openmp --enable-strict-barrier --disable-long-double CC="gcc-9 -fsanitize=undefined,address -fno-sanitize=float-divide-by-zero -fno-omit-frame-pointer"
make

Expand Down
2 changes: 1 addition & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Package: data.table
Version: 1.14.3
Version: 1.14.5
Title: Extension of `data.frame`
Authors@R: c(
person("Matt","Dowle", role=c("aut","cre"), email="mattjdowle@gmail.com"),
Expand Down
6 changes: 3 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ some:

.PHONY: clean
clean:
$(RM) data.table_1.14.3.tar.gz
$(RM) data.table_1.14.5.tar.gz
$(RM) src/*.o
$(RM) src/*.so

Expand All @@ -28,7 +28,7 @@ build:

.PHONY: install
install:
$(R) CMD INSTALL data.table_1.14.3.tar.gz
$(R) CMD INSTALL data.table_1.14.5.tar.gz

.PHONY: uninstall
uninstall:
Expand All @@ -40,7 +40,7 @@ test:

.PHONY: check
check:
_R_CHECK_CRAN_INCOMING_REMOTE_=false $(R) CMD check data.table_1.14.3.tar.gz --as-cran --ignore-vignettes --no-stop-on-test-error
_R_CHECK_CRAN_INCOMING_REMOTE_=false $(R) CMD check data.table_1.14.5.tar.gz --as-cran --ignore-vignettes --no-stop-on-test-error

.PHONY: revision
revision:
Expand Down
9 changes: 8 additions & 1 deletion NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

**Benchmarks are regularly updated: [here](https://h2oai.github.io/db-benchmark/)**

# data.table [v1.14.3](https://github.com/Rdatatable/data.table/milestone/20) (in development)
# data.table [v1.14.5](https://github.com/Rdatatable/data.table/milestone/20) (in development)

## NEW FEATURES

Expand Down Expand Up @@ -608,6 +608,13 @@
17. `update.dev.pkg()` has been renamed `update_dev_pkg()` to get out of the way of the `stats::update` generic function, [#5421](https://github.com/Rdatatable/data.table/pull/5421). This is a utility function which upgrades the version of `data.table` to the latest commit in development which has also passed all tests. As such we don't expect any backwards compatibility concerns.


# data.table [v1.14.4](https://github.com/Rdatatable/data.table/milestone/26?closed=1)

## NOTES

1. gcc 12.1 (May 2022) now detects and warns about an always-false condition in `fread` which caused a small efficiency saving never to be invoked. Thanks to CRAN for testing latest versions of compilers.


# data.table [v1.14.2](https://github.com/Rdatatable/data.table/milestone/24?closed=1) (27 Sep 2021)

## NOTES
Expand Down
37 changes: 19 additions & 18 deletions src/fread.c
Original file line number Diff line number Diff line change
Expand Up @@ -1284,25 +1284,22 @@ int freadMain(freadMainArgs _args) {
while (*nastr) {
if (**nastr == '\0') {
blank_is_a_NAstring = true;
// if blank is the only one, as is the default, clear NAstrings so that doesn't have to be checked
if (nastr==NAstrings && nastr+1==NULL) NAstrings=NULL;
nastr++;
continue;
} else {
const char *ch = *nastr;
size_t nchar = strlen(ch);
if (isspace(ch[0]) || isspace(ch[nchar-1]))
STOP(_("freadMain: NAstring <<%s>> has whitespace at the beginning or end"), ch);
if (strcmp(ch,"T")==0 || strcmp(ch,"F")==0 ||
strcmp(ch,"TRUE")==0 || strcmp(ch,"FALSE")==0 ||
strcmp(ch,"True")==0 || strcmp(ch,"False")==0)
STOP(_("freadMain: NAstring <<%s>> is recognized as type boolean, this is not permitted."), ch);
if ((strcmp(ch,"1")==0 || strcmp(ch,"0")==0) && args.logical01)
STOP(_("freadMain: NAstring <<%s>> and logical01=%s, this is not permitted."), ch, args.logical01 ? "TRUE" : "FALSE");
Copy link
Member

Choose a reason for hiding this comment

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

slightly confused by the templating in the string here am I missing something? in the error case logical01 is always TRUE?

Copy link
Member Author

@mattdowle mattdowle Oct 7, 2022

Choose a reason for hiding this comment

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

good spot. agree and logical01=TRUE, this could be in the string directly and the ternary isn't needed.

char *end;
errno = 0;
(void)strtod(ch, &end); // careful not to let "" get to here (see continue above) as strtod considers "" numeric
Copy link
Member

@MichaelChirico MichaelChirico Oct 7, 2022

Choose a reason for hiding this comment

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

comment looks stale now that 'continue' is removed

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed. good spot.

if (errno==0 && (size_t)(end - ch) == nchar) any_number_like_NAstrings = true;
}
const char *ch = *nastr;
size_t nchar = strlen(ch);
if (isspace(ch[0]) || isspace(ch[nchar-1]))
STOP(_("freadMain: NAstring <<%s>> has whitespace at the beginning or end"), ch);
if (strcmp(ch,"T")==0 || strcmp(ch,"F")==0 ||
strcmp(ch,"TRUE")==0 || strcmp(ch,"FALSE")==0 ||
strcmp(ch,"True")==0 || strcmp(ch,"False")==0)
STOP(_("freadMain: NAstring <<%s>> is recognized as type boolean, this is not permitted."), ch);
if ((strcmp(ch,"1")==0 || strcmp(ch,"0")==0) && args.logical01)
STOP(_("freadMain: NAstring <<%s>> and logical01=%s, this is not permitted."), ch, args.logical01 ? "TRUE" : "FALSE");
char *end;
errno = 0;
(void)strtod(ch, &end); // careful not to let "" get to here (see continue above) as strtod considers "" numeric
if (errno==0 && (size_t)(end - ch) == nchar) any_number_like_NAstrings = true;
nastr++;
}
disabled_parsers[CT_BOOL8_N] = !args.logical01;
Expand All @@ -1325,6 +1322,10 @@ int freadMain(freadMainArgs _args) {
DTPRINT(_(" show progress = %d\n"), args.showProgress);
DTPRINT(_(" 0/1 column will be read as %s\n"), args.logical01? "boolean" : "integer");
}
if (*NAstrings==NULL || // user sets na.strings=NULL
(**NAstrings=='\0' && *(NAstrings+1)==NULL)) { // user sets na.strings=""
NAstrings=NULL; // clear NAstrings to save end_NA_string() dealing with these cases (blank_is_a_NAstring was set to true above)
}

stripWhite = args.stripWhite;
skipEmptyLines = args.skipEmptyLines;
Expand Down
2 changes: 1 addition & 1 deletion src/init.c
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,6 @@ SEXP initLastUpdated(SEXP var) {

SEXP dllVersion() {
// .onLoad calls this and checks the same as packageVersion() to ensure no R/C version mismatch, #3056
return(ScalarString(mkChar("1.14.3")));
return(ScalarString(mkChar("1.14.5")));
}