-
Notifications
You must be signed in to change notification settings - Fork 60
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
Align argument types and format strings #524
Conversation
@@ -5,6 +5,7 @@ | |||
#include <cstring> | |||
#include <sstream> | |||
#include <string> | |||
#include <tuple> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't say that I completely understand why the need for this was surfaced by the changes in the content and usage of r_utils.h
. But I'm also not sure that I really need to get to the bottom of it.
#ifndef R_PRIdXLEN_T | ||
# ifdef LONG_VECTOR_SUPPORT | ||
# define R_PRIdXLEN_T "td" | ||
# else | ||
# define R_PRIdXLEN_T "d" | ||
# endif | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who/what is going to define LONG_VECTOR_SUPPORT
🤔 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it comes via cpp11/R.hpp
, which includes Rinternals.h
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is coming from the R headers, you are allowed to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me, much better than casting. But just to make sure, would you mind adding these to the CI matrix, temporarily?
- windows 4.2
- windows R-devel
Windows 4.2, because it uses yet another compiler that we do not test for currently, and R-devel because that's where the recent changes are.
I am still not entirely sure if it is going to work without a warning on older Linux, e.g. Centos/RedHat 7, I'll check it soon.
EDIT: seems fine on CentOS 7 as well (with gcc 4.8.5!), with R-release and R-devel.
Checking on Windows with r-devel was revealing. The existing fixes all hold up, but I get some new hits in a totally different file, so I'm happy to learn about this now! I also sent everything to win-builder (devel, release, oldrel) and get completely consistent results: the new warnings seen below on devel, clean pass on release and oldrel.
Update: I now see these additional warnings also in the CRAN results for windows + r-devel. I think that hadn’t shown up yet when I first got the email (or else I just overlooked it / assumed all flavours were complaining about the same things). |
Closes #523