Skip to content
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

Remove BH dependency #297

Merged
merged 13 commits into from
Apr 1, 2021
Merged

Remove BH dependency #297

merged 13 commits into from
Apr 1, 2021

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Mar 24, 2021

Replaced boost/optional with https://github.com/akrzemi1/Optional.

parse_http_date_string() used boost/date_time; replaced this with sscanf() and timegm(). But timegm() isn't available on mingw, and the first workaround I tried caused the DLL not to load during CI. The final workaround reimplements timegm() using Win32 APIs.

See also r-lib/later#147

@jcheng5 jcheng5 marked this pull request as ready for review March 24, 2021 02:47
@jcheng5 jcheng5 requested a review from wch March 24, 2021 02:47
src/utils.h Outdated Show resolved Hide resolved
@jcheng5
Copy link
Member Author

jcheng5 commented Mar 30, 2021

get_time does seem much, much better! Thanks for the tip.

For testing, I tried replacing the locale ("C") with a locale that is not installed, and that caused an exception; the try/catch(...) catches that exception. I also tried replacing the locale with a valid locale in a different language, and that caused the parsing to fail gracefully (which confirms that get_time does respect the imbued locale).

@jcheng5 jcheng5 requested a review from wch March 30, 2021 19:50
@wch
Copy link
Collaborator

wch commented Mar 30, 2021

It's nice that you can remove all of the manual conversion stuff up above by setting the locale for the stream object.

Just curious: do you know what it does with the day of week (like "Tue")? Does that get thrown away, or does it check for a mismatch?

@wch
Copy link
Collaborator

wch commented Mar 30, 2021

To answer my own question: the day of week gets put in the tm object:

cpp11::cpp_source(
  code = '
#include <cpp11.hpp>
#include <sstream>
#include <iomanip>

using namespace cpp11;

[[cpp11::register]]
cpp11::integers datetest(std::string date) {
  std::tm t = {};

  std::istringstream date_ss(date);
  date_ss >> std::get_time(&t, "%a, %d %b %Y %H:%M:%S GMT");
  return writable::integers({
    t.tm_isdst,
    t.tm_yday,
    t.tm_wday,
    t.tm_year,
    t.tm_mon,
    t.tm_mday,
    t.tm_hour,
    t.tm_min,
    t.tm_sec
  });
}
')

datetest("Tue, 30 Mar 2021 15:14:00 GMT")
#> [1]   0   0   2 121   2  30  15  14   0
datetest("Wed, 30 Mar 2021 15:14:00 GMT")
#> [1]   0   0   3 121   2  30  15  14   0

But I think it gets ignored for date-time comparisons later on.

DESCRIPTION Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@wch
Copy link
Collaborator

wch commented Mar 30, 2021

As of gcc 4.9.3 (which I believe is the version included with rtools 3.5), get_time is not implemented. This is item 27.7 in the table here: https://gcc.gnu.org/onlinedocs/gcc-4.9.3/libstdc++/manual/manual/status.html

@jcheng5 jcheng5 requested a review from wch March 30, 2021 22:42
src/utils.cpp Show resolved Hide resolved
src/utils.cpp Outdated Show resolved Hide resolved
src/utils.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@wch wch left a comment

Choose a reason for hiding this comment

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

One piece of code that should be changed, but otherwise looks good.

@jcheng5 jcheng5 requested a review from wch April 1, 2021 08:03
@wch wch merged commit 184a1bb into master Apr 1, 2021
@wch wch deleted the remove-bh branch April 1, 2021 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants