std::get_time and strptime for the truly unfortunate. (Closes #9) #11

Merged
merged 12 commits into from Jan 7, 2017

Projects

None yet

2 participants

@dcdillon
Contributor

This adds a backport of std::get_time (in the std_backports namespace) and shores up the strptime() implementation based on std::get_time.

Windows here we come!

@dcdillon dcdillon changed the title from std::get_time and strptime for the truly unfortunate. to std::get_time and strptime for the truly unfortunate. (Closes #9) Dec 31, 2016
src/time_zone_format.cc
@@ -31,27 +31,43 @@
#if !HAS_STRPTIME
#include <iomanip>
#include <sstream>
+#include <get_time.h>
@eddelbuettel
eddelbuettel Jan 3, 2017 Owner

Can we make the include conditional on being compiled with MinGW? Just to minimise side-effects...

src/time_zone_format.cc
-namespace detail {
-
-namespace {
+namespace RcppCCTZ {
@eddelbuettel
eddelbuettel Jan 3, 2017 Owner

And maybe keep the existing namespace?

src/time_zone_format.cc
-#else
- dp = strptime(dp, fmt, tm);
-#endif
+ dp = RcppCCTZ::strptime(dp, fmt, tm);
@eddelbuettel
eddelbuettel Jan 3, 2017 Owner

Maybe standard strptime() most of the time and our if and when we have no alternative?

@dcdillon dcdillon Moving things around a bit. Temporary commit to move the changes from…
… my desk to my bedroom because my network isn't quite right.
bdcc2b3
src/time_zone_format.cc
@@ -12,6 +12,8 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+#define __MINGW64__
@eddelbuettel
eddelbuettel Jan 6, 2017 Owner

That one probably needs to go eventually.

src/time_zone_format.cc
@@ -516,7 +523,7 @@ const char* ParseSubSeconds(const char* dp,
// Parses a string into a std::tm using strptime(3).
const char* ParseTM(const char* dp, const char* fmt, std::tm* tm) {
if (dp != nullptr) {
- dp = RcppCCTZ::strptime(dp, fmt, tm);
+ dp = cctz::detail::strptime(dp, fmt, tm);
@eddelbuettel
eddelbuettel Jan 6, 2017 Owner

That's a better namespace -- +1

@dcdillon
Contributor
dcdillon commented Jan 6, 2017

Lol read the commit comment

@eddelbuettel
Owner

Ouch. Damn ipOverPower messing with you?

@eddelbuettel
Owner

I am not sure I like the last change set. I don;t think I want to change away from %F %T by default. I'd rather condition.

But it looks like you have a build failure to catch anyway?

@dcdillon
Contributor
dcdillon commented Jan 6, 2017

Gotcha...no more time tonight. Will have to address in the morning. For now, everything should work correctly on every platform.

@eddelbuettel eddelbuettel merged commit de8629f into eddelbuettel:master Jan 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment