Feature/temporal types #58
Conversation
…ct treatment of NA for all types
…are different between the two
…of 0-length types
… check for double specification of timezone
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
==========================================
- Coverage 100% 99.94% -0.06%
==========================================
Files 1 14 +13
Lines 97 1849 +1752
==========================================
+ Hits 97 1848 +1751
- Misses 0 1 +1
Continue to review full report at Codecov.
|
|
Massive indeed. I left some quick and simple comments. I will not claim to have read each line in each of the code lines. Let me run some tests... |
ChangeLog
Outdated
|
|
||
| * .travis.yml: Load development version of RcppCCTZ from GitHub | ||
|
|
||
| 2019-11-27 Leonardo Silvestri <lsilvestri@ztsdb.org> |
There was a problem hiding this comment.
More current date, maybe?
(Also, generally, ChangeLog is per "chunk" of changes, similar to git logs, maybe a a little more coarse. Here, it is of course a collection of a LOT of changes so may as well say so. Your code, your reference -- whatever feels rights.)
There was a problem hiding this comment.
I changed the date part, and added two header lines, see 18bc559
DESCRIPTION
Outdated
| Version: 0.2.4.2 | ||
| Date: 2019-11-21 | ||
| Version: 0.2.4.3 | ||
| Date: 2019-11-28 |
|
|
||
| // The MIT License (MIT) | ||
| // | ||
| // Copyright (c) 2015, 2016 Howard Hinnant |
There was a problem hiding this comment.
Hahhahhaa. I was wondering when I would have to give in an include his. He is unavoidable :) And good, this is now in C++20 as well.
(Should we farm it out to another package though to be more widely useable? Just a thought....)
src/Makevars
Outdated
| ## We want C++11 | ||
| CXX_STD = CXX11 | ||
| PKG_CXXFLAGS = -I../inst/include | ||
| PKG_LIBS = -lstdc++ -ldl |
There was a problem hiding this comment.
I don't think we want/need those for PKG_LIBS. They should be added automatically.
src/duration.cpp
Outdated
|
|
||
|
|
||
| RcppExport SEXP duration_from_string(SEXP s) { | ||
| try { |
There was a problem hiding this comment.
We can do possibly one better here by still relying on // [[Rcpp::export]] to get automatic type conversion AND the try/catch wrapper. One trick is to use // [[Rcpp::export(.duration_from_string)]] which does all the work for us, but still does not make the function 'public' in the "need a manual page for it too" sense.
There was a problem hiding this comment.
Ah, great, didn't know that. That is better and safer. Will take a look at that.
There was a problem hiding this comment.
As discussed on-line, I ran into a brick wall trying to implement this in a test case. Which is puzzling, but enough to put this off for now. Filed under 'mysteries happening once S4 classes enter'. Works fine in simpler packages.
| ++s; | ||
| if (s + 5 > e || !isdigit(s[0]) || !isdigit(s[1]) || | ||
| s[2] != ':' || !isdigit(s[3]) || !isdigit(s[4])) { | ||
| throw std::range_error("cannot parse nanoduration"); |
There was a problem hiding this comment.
I am wondering whether this file should
- get a
try/catchblock it currently does not have - use
Rcpp::stop()instead ofthrow
The preference for Rcpp::stop() would apply to the other files too, of course.
There was a problem hiding this comment.
Not sure. This is a low level function with no call to any of the R interface and when it is called it is protected by a try/catch block in RcppExport SEXP duration_from_string(SEXP s). You could see it as a library function and like the STL functions, it can throw. But one has to believe the exception mechanism will handle things correctly or else one would never use any C++: more specifically, after the catch in duration_from_string we have to be in a correct state or else C++ is broken (and we're not in a weird case like throwing from inside a constructor with partially allocated stuff).
On the other hand, I'm not sure of the subtleties before returning to R. So maybe Rcpp::stop should replace the forward_exception_to_r? I will look into the source code of Rcpp::stop to see what it's doing and get a better feel for some of the issues we could encounter. We could certainly replace all the forward_expection_to_r if Rcpp::stop is better.
There was a problem hiding this comment.
It's complicated. You and I know the code will only be called from out try/catch. But as it stands, it is a naked throw. Which is a minor smell. But heck, as we said, "we know" and we could just add a line of documentation.
As for the Rcpp::stop() -- I tend to use it everywhere (when Rcpp is used) instead of a plain throw because it has some associated code with it that should generally be beneficial and not impose cost. Call it style preference. Effectively the difference may not matter. Just how I tend to indent C/C++/R code by four spaces but everybody using RStudio comes at me with two spaces. Oh well :)
eddelbuettel
force-pushed
the
feature/temporal_types
branch
from
8ebde52
to
108ea4f
Compare
OK, this is a huge pull request.
I think the code is complete, and we've got 100% coverage.
There's still no vignette and I'll be working on that, but I thought in the mean time that it would be worthwhile going through a code review (which is also a great way for me to take a step back and look at the new code in its whole).
I will explain the main changes going in. Some are obvious (e.g. new types), so are more subtle like relaxed format, etc.