C level function that returns the offset at a given time-point in a given timezone

[lsilvest]

For nanotime's period type, we need access to the timezone data so that adding and subtracting a period is correctly handled. The operation look like this:

    nt <- nanotime("2018-05-01T00:00:00.000000000-04:00")
    p  <- as.period("4m")
    tz <- "America/New_York"
    expected <- nanotime("2018-01-01T00:00:00.000000000-05:00")
    checkIdentical(minus(nt, p, tz), expected)

In order to do this, we need a function to get the offset for a particular timezone at a given time-point, the function getOffset.

This function is then registered as a C function using R_RegisterCCallable so that it can be called at C level by another package.

This is an ad-hoc change for nanotime, but it could make sense to add the set of functions exported in R (or one could wait until there's a specific request of course!).

[eddelbuettel]

Nice PR. Please also add ChangeLog and NEWS as usual. Maybe even a test :)

[eddelbuettel]

int64_t, maybe, or uint64_t, or ...int128_t ? long long is less determined across architectures methinks.

Also: do we really need more than, say uint64_t ?

[lsilvest]

Indeed, I pulled that from older code; I have changed that to std::int_fast64_t as that is what's used throughout the library.

[eddelbuettel]

I tend to use plain int64_t more but I trust the CCTZ folks on this :)

[eddelbuettel]

Registering at the C level for other functions is such a good idea that I am really embarrassed I did not think of it myself. There is probably scope for one or two more so yes, let's get this started!

[lsilvest]

Yes, will try to think about a coherent set that would make sense. Shall I create an issue to keep track of this?

[eddelbuettel]

Yes please! Issues are best for that.

[eddelbuettel]

Nice work.

[lsilvest]

Thanks; I have added a NEWS and ChangeLog and also bumped the minor version number from 0.2.6 to 0.2.7.

[lsilvest]

Test is a little difficult, as we can't directly call this function from R, so we'd need a C++ only test. Not at all sure how to do this; shall we leave it as-is right now and look into this when we add additional C low-level routines?

[eddelbuettel]

Good point about the test (esp from another package) -- that is doable but not worth it.

But can we test the basic offset computation? Maybe from a wrapper?

[lsilvest]

I think the only way we can test the call from R is if we make an R function for it or for a function that calls it (which is for example what nanotime does. Then we would have to document it at the R level, right? Or can we write a wrapper without exporting it at the package level (which we could call something like _RcppCCTZ_test_exported_C_functions and would simply return TRUE or FALSE)?

[eddelbuettel]

You probably have thought much much harder about this particular question than I have.

So if we can't, we can't. The tests can of course come from the nanotime side too.

[lsilvest]

nanotime makes that call and tests it with a few offsets, so I think it's OK for this function. That said, I'm not sure what's the best way to handle this issue that will come up again if additional functions are exported. Will think about that more...

[eddelbuettel]

In Rcpp we went balistic (as we had to ...) and within the unit testing framework build and a run a client package. It's all just scripting ...

[lsilvest]

Seems the way to go: will look at how it's done in Rccp.

[eddelbuettel]

Not sure we need to here. Tests over from nanotime seems fine and legit given that we own both ends.

[eddelbuettel]

Don't mean to be a pain with ChangeLog and inst/NEWS.Rd. If you prefer that I do it that's fine too. Just let me know...

[eddelbuettel]

I tend not to make releases from PRs quite yet. Let's make tthat 0.2.6.1, and adjust the ChangeLog to one of those 'Roll minor version' entries as well?

[lsilvest]

Absolutely, that makes sense.

[lsilvest]

Btw, the reason a version change is good is that nanotime will depend on it in order for period to work correctly, so it's useful that we can request that specific version. So 0.2.6.1 works perfectly :)

[eddelbuettel]

I have been using that three-digits-for-releases and four for interim versions that are still differentiable for a really long time. It really works. :) And also with the package_version S3 class (and/or the dpkg package comparison).

[eddelbuettel]

Sorry -- that should into inst/NEWS.Rd.

[lsilvest]

Ah, missed the location!

[eddelbuettel]

'Roll minor version'

[eddelbuettel]

Looks like this still says 0.2.7. I'll clean it up in a minute ...

[eddelbuettel]

Or maybe I am caught in a 'you are reviewing, finish this' loop at GH...

[eddelbuettel]

I edited the ChangeLog and pushed so make sure you pull if you need to make further changes.

Thanks for the PR -- very nice!

[lsilvest]

Excellent, thanks!