Skip to content

mingw I64 warnings #4073

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.

Sign up for GitHub

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

Merged
mattdowle merged 2 commits into from
Dec 5, 2019
Merged

mingw I64 warnings #4073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fb9c72
added isolating layer dt_stdio.h
mattdowle Nov 22, 2019
3c37be8
passes win-builder r-devel ATC
mattdowle Nov 24, 2019
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
2 changes: 0 additions & 2 deletions src/assign.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
#include "data.table.h"
#include <Rdefines.h>
#include <Rmath.h>

static void finalizer(SEXP p)
{
Expand Down
2 changes: 1 addition & 1 deletion src/data.table.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
#include "dt_stdio.h" // PRId64 and PRIu64
#include <R.h>
#define USE_RINTERNALS
#include <Rinternals.h>
// #include <signal.h> // the debugging machinery + breakpoint aidee
// raise(SIGINT);
#include <stdint.h> // for uint64_t rather than unsigned long long
#include <inttypes.h> // for PRId64 and PRIu64
#include <stdbool.h>
#include "myomp.h"
#include "types.h"
Expand Down
34 changes: 34 additions & 0 deletions src/dt_stdio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@

// It seems that latest min64-w64 compiler needs some help to avoid warnings; #4064
// It appears that its inttypes.h defines PRId64 to be I64 on Windows but then warns that
// I64 is not C99 too. Which is correct, but we know, and we don't want any warnings, and
// we can't control settings on CRAN. So here we try and isolate ourselves from all scenarios.
// There is a lot online about this, but these were a few that were most useful:
// https://stackoverflow.com/questions/23718110/error-unknown-conversion-type-character-l-in-format-scanning-long-long
// https://gitlab.freedesktop.org/nacho.garglez/cerbero/commit/4ec6bfdc1db1e4bc8d4278f53ad295af1efe89fb
// https://github.com/GNOME/glib/commit/98a0ab929d8c59ee27e5f470f11d077bb6a56749#diff-969b60ad3d206fd45c208e266ccfed38
// https://sourceforge.net/p/mingw-w64/wiki2/gnu%20printf/
// Warning that __USE_MINGW_ANSI_STDIO could be deprecated:
// https://mingw-w64-public.narkive.com/aQDJHqCv/how-to-printf-64-bit-integer-in-64-bit-compiler-in-strict-iso-mode-with-all-warnings-enabled
// and then actual deprecation (with complaints) earlier in 2019 here:
// https://osdn.net/projects/mingw/lists/archive/users/2019-January/000202.html
// But I can't find _mingw.h. I can find mingw.h but not advice within it:
// https://github.com/git/git/blob/master/compat/mingw.h#L448
// The deprecation about I64 doesn't seem to take into account that lld throws the warning in MinGW:
// warning: unknown conversion type character 'l' in format [-Wformat=]
// So let's see if the approach below works for now.
// If we need to make changes in future, we can do it here in one place.

#ifndef DT_STDIO_H
#define DT_STDIO_H
#if defined(__MINGW32__) || (defined __MINGW64__)
#define __USE_MINGW_ANSI_STDIO 1
#include <stdio.h>
#define PRId64 "lld"
#define PRIu64 "llu"
#else
#include <stdio.h>
#include <inttypes.h>
// let inttypes.h decide: lld on Linux/C99; I64 on Windows Visual Studio for example
#endif
#endif
8 changes: 4 additions & 4 deletions src/fread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
#ifndef dt_FREAD_H
#define dt_FREAD_H
#include <stdint.h> // uint32_t
#include <inttypes.h> // PRId64
#include <stdlib.h> // size_t
#include <stdbool.h> // bool
#include "dt_stdio.h" // PRId64
#include <stdint.h> // uint32_t
#include <stdlib.h> // size_t
#include <stdbool.h> // bool
#include "myomp.h"
#ifdef DTPY
#include "py_fread.h"
Expand Down
12 changes: 4 additions & 8 deletions src/froll.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>
#include <Rdefines.h>
#include "data.table.h"

/* fast rolling mean - router
Expand Down Expand Up @@ -49,7 +45,7 @@ void frollmean(unsigned int algo, double *x, uint64_t nx, ans_t *ans, int k, int
*/
void frollmeanFast(double *x, uint64_t nx, ans_t *ans, int k, double fill, bool narm, int hasna, bool verbose) {
if (verbose)
snprintf(end(ans->message[0]), 500, "%s: running for input length %"PRIu64", window %d, hasna %d, narm %d\n", __func__, (uint64_t)nx, k, hasna, (int)narm);
snprintf(end(ans->message[0]), 500, "frollmeanFast: running for input length %"PRIu64", window %d, hasna %d, narm %d\n", (uint64_t)nx, k, hasna, (int)narm);
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico Dec 8, 2019

Choose a reason for hiding this comment

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

For translation purposes, it will be simpler to replace __func__ with "frollmeanFast", then only the format message has to be translated, rather than translating essentially the same things a bunch of times.

Took the idea from base which does this often.

I'll update this in the translation branch shortly

Copy link
Copy Markdown
Member Author

@mattdowle mattdowle Dec 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ok. But does that pass the new mingw-w64 compiler in rtools4 (win-builder ATC)? The potential compiler glitch might pertain to the %s and be unrelated to the __func__. Which is why I went straight for not using %s at all to save time to see if that passed, and it did to my surprise. Before putting back the %s we have to check whether the problem is %s or __func__ and raise the issue with mingw-w64 team.

Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico Dec 8, 2019

Choose a reason for hiding this comment

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

makes sense. is there such a test in our GL CI pipeline?

Copy link
Copy Markdown
Member Author

@mattdowle mattdowle Dec 9, 2019

Choose a reason for hiding this comment

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

not sure how hard that is to setup, @jangorecki? Maybe it's just a case of pointing to (new) rtools40, or could be more involved.

Copy link
Copy Markdown
Member Author

@mattdowle mattdowle Dec 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Update: #3935 (comment); i.e. yes it's specifically the __func__ with the problem and not the %s.

Copy link
Copy Markdown
Member

@jangorecki jangorecki Dec 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@mattdowle I missed your previous comment. Good we know that __func__ was causing problem when used with int64 printing. If rtools40 would help about this, do we want to use __func__ together with int64 printing again? It is just a windows specific compilation warning so not really an issue.

Copy link
Copy Markdown
Member Author

@mattdowle mattdowle Dec 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@jangorecki by 'if rtools40 would help about this' do you mean if we report to mingw-w64 team, they accept it as bug, they fix the bug, and then Rtools4.0 upgrades to the fixed version of gcc, do we then want to put the __func__ back? If so, I guess yes, because the code in this file is currently confusing because it uses __func__ everywhere except those 4 calls that include the int64 too. It's strange to have that difference so it has to be commented to explain, and I suppose we need something in CRAN_Release.cmd too to catch that from coming back in future. Is GLCI using Rtools4.0 yet and does GLCI search the install log on windows to catch compiler warnings like this one?

Copy link
Copy Markdown
Member

@jangorecki jangorecki Dec 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We use 3.5.0.4 on gitlab builds. rtools40 is still experimental, will just test how it works but not yet upgrade our environment.

long double w = 0.0; // sliding window aggregate
bool truehasna = hasna>0; // flag to re-run with NA support if NAs detected
if (!truehasna) {
Expand Down Expand Up @@ -138,7 +134,7 @@ void frollmeanFast(double *x, uint64_t nx, ans_t *ans, int k, double fill, bool
*/
void frollmeanExact(double *x, uint64_t nx, ans_t *ans, int k, double fill, bool narm, int hasna, bool verbose) {
if (verbose)
snprintf(end(ans->message[0]), 500, "%s: running in parallel for input length %"PRIu64", window %d, hasna %d, narm %d\n", __func__, (uint64_t)nx, k, hasna, (int)narm);
snprintf(end(ans->message[0]), 500, "frollmeanExact: running in parallel for input length %"PRIu64", window %d, hasna %d, narm %d\n", (uint64_t)nx, k, hasna, (int)narm);
for (int i=0; i<k-1; i++) { // fill partial window only
ans->dbl_v[i] = fill;
}
Expand Down Expand Up @@ -252,7 +248,7 @@ void frollsum(unsigned int algo, double *x, uint64_t nx, ans_t *ans, int k, int
}
void frollsumFast(double *x, uint64_t nx, ans_t *ans, int k, double fill, bool narm, int hasna, bool verbose) {
if (verbose)
snprintf(end(ans->message[0]), 500, "%s: running for input length %"PRIu64", window %d, hasna %d, narm %d\n", __func__, (uint64_t)nx, k, hasna, (int)narm);
snprintf(end(ans->message[0]), 500, "frollsumFast: running for input length %"PRIu64", window %d, hasna %d, narm %d\n", (uint64_t)nx, k, hasna, (int)narm);
long double w = 0.0;
bool truehasna = hasna>0;
if (!truehasna) {
Expand Down Expand Up @@ -336,7 +332,7 @@ void frollsumFast(double *x, uint64_t nx, ans_t *ans, int k, double fill, bool n
}
void frollsumExact(double *x, uint64_t nx, ans_t *ans, int k, double fill, bool narm, int hasna, bool verbose) {
if (verbose)
snprintf(end(ans->message[0]), 500, "%s: running in parallel for input length %"PRIu64", window %d, hasna %d, narm %d\n", __func__, (uint64_t)nx, k, hasna, (int)narm);
snprintf(end(ans->message[0]), 500, "frollsumExact: running in parallel for input length %"PRIu64", window %d, hasna %d, narm %d\n", (uint64_t)nx, k, hasna, (int)narm);
for (int i=0; i<k-1; i++) {
ans->dbl_v[i] = fill;
}
Expand Down
12 changes: 4 additions & 8 deletions src/frolladaptive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>
#include <Rdefines.h>
#include "data.table.h"

/* fast adaptive rolling mean - router
Expand Down Expand Up @@ -30,7 +26,7 @@ void fadaptiverollmean(unsigned int algo, double *x, uint64_t nx, ans_t *ans, in
*/
void fadaptiverollmeanFast(double *x, uint64_t nx, ans_t *ans, int *k, double fill, bool narm, int hasna, bool verbose) {
if (verbose)
snprintf(end(ans->message[0]), 500, "%s: running for input length %"PRIu64", hasna %d, narm %d\n", __func__, (uint64_t)nx, hasna, (int) narm);
snprintf(end(ans->message[0]), 500, "fadaptiverollmeanFast: running for input length %"PRIu64", hasna %d, narm %d\n", (uint64_t)nx, hasna, (int) narm);
bool truehasna = hasna>0; // flag to re-run if NAs detected
long double w = 0.0;
double *cs = malloc(nx*sizeof(double)); // cumsum vector, same as double cs[nx] but no segfault
Expand Down Expand Up @@ -115,7 +111,7 @@ void fadaptiverollmeanFast(double *x, uint64_t nx, ans_t *ans, int *k, double fi
*/
void fadaptiverollmeanExact(double *x, uint64_t nx, ans_t *ans, int *k, double fill, bool narm, int hasna, bool verbose) {
if (verbose)
snprintf(end(ans->message[0]), 500, "%s: running in parallel for input length %"PRIu64", hasna %d, narm %d\n", __func__, (uint64_t)nx, hasna, (int) narm);
snprintf(end(ans->message[0]), 500, "fadaptiverollmeanExact: running in parallel for input length %"PRIu64", hasna %d, narm %d\n", (uint64_t)nx, hasna, (int) narm);
bool truehasna = hasna>0; // flag to re-run if NAs detected
if (!truehasna || !narm) { // narm=FALSE handled here as NAs properly propagated in exact algo
#pragma omp parallel for num_threads(getDTthreads())
Expand Down Expand Up @@ -219,7 +215,7 @@ void fadaptiverollsum(unsigned int algo, double *x, uint64_t nx, ans_t *ans, int
}
void fadaptiverollsumFast(double *x, uint64_t nx, ans_t *ans, int *k, double fill, bool narm, int hasna, bool verbose) {
if (verbose)
snprintf(end(ans->message[0]), 500, "%s: running for input length %"PRIu64", hasna %d, narm %d\n", __func__, (uint64_t)nx, hasna, (int) narm);
snprintf(end(ans->message[0]), 500, "fadaptiverollsumFast: running for input length %"PRIu64", hasna %d, narm %d\n", (uint64_t)nx, hasna, (int) narm);
bool truehasna = hasna>0;
long double w = 0.0;
double *cs = malloc(nx*sizeof(double));
Expand Down Expand Up @@ -299,7 +295,7 @@ void fadaptiverollsumFast(double *x, uint64_t nx, ans_t *ans, int *k, double fil
}
void fadaptiverollsumExact(double *x, uint64_t nx, ans_t *ans, int *k, double fill, bool narm, int hasna, bool verbose) {
if (verbose)
snprintf(end(ans->message[0]), 500, "%s: running in parallel for input length %"PRIu64", hasna %d, narm %d\n", __func__, (uint64_t)nx, hasna, (int) narm);
snprintf(end(ans->message[0]), 500, "fadaptiverollsumExact: running in parallel for input length %"PRIu64", hasna %d, narm %d\n", (uint64_t)nx, hasna, (int) narm);
bool truehasna = hasna>0;
if (!truehasna || !narm) {
#pragma omp parallel for num_threads(getDTthreads())
Expand Down
2 changes: 1 addition & 1 deletion src/fwrite.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
#include "dt_stdio.h"
#include <errno.h>
#include <unistd.h> // for access()
#include <fcntl.h>
#include <stdbool.h> // true and false
#include <stdint.h> // INT32_MIN
#include <inttypes.h> // PRId64
#include <math.h> // isfinite, isnan
#include <stdlib.h> // abs
#include <string.h> // strlen, strerror
Expand Down