NotificationsYou must be signed in to change notification settings
Fork219
Star781

added format() methods for Date and Datetime#599

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

eddelbuettel merged 5 commits intomasterfromfeature/format_dates

Nov 28, 2016

Merged

added format() methods for Date and Datetime#599

eddelbuettel merged 5 commits intomasterfromfeature/format_dates

Nov 28, 2016

Conversation

Copy link

Member

eddelbuettel commentedNov 27, 2016

also rolled dev version in config.h

also addsoperator<<() which doesn't seem to get picked up by compiler, ie we get the underlying date number or seconds since epoch.

added format() methods for Date and Datetime

08a2cff

rolled dev version in config.h

Copy link

Contributor

nathan-russell commentedNov 27, 2016

This looks good to me; my only suggestion would be possibly dropping the trailing<< std::endl;, as there may be cases where the extra newline and call toflush is not desirable.

Copy link

MemberAuthor

eddelbuettel commentedNov 27, 2016

Thanks for looking at these. These were so far mostly for seeing what it would -- where I really want them is in the vector case of the new classes. And yes, the forced newline would be annoying there. For an individual item it may still make sense. Not sure yet how to balance it, and when to line break for vectors (every N maybe?).

Copy link

MemberAuthor

eddelbuettel commentedNov 27, 2016

Also annoying:

R> library(Rcpp)R> cppFunction('void foo(Date d) { Rcpp::Rcout << d << std::endl; }')R> foo(Sys.Date())17132R> cppFunction('void bar(Date d) { Rcpp::Rcout << d.format() << std::endl; }')R> bar(Sys.Date())2016-11-27R>

I'd love forfoo() to do whatbar() does.

Copy link

MemberAuthor

eddelbuettel commentedNov 27, 2016

There is something else going on. I don't even think the newoperator<<() gets called there.

coatless reviewed

Nov 27, 2016

View reviewed changes

inst/include/Rcpp/date_datetime/Datetime.h Outdated

		returnstd::string(txtsec);
		}

		inline std::ostream &operator<<(std::ostream & s)const {

Copy link

Contributor

coatlessNov 27, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Might be naive on my part, but:Rcpp::Rcout orRcpp::Rstreambuf instead ofstd::ostream?

Copy link

MemberAuthor

eddelbuettelNov 27, 2016

Choose a reason for hiding this comment

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

No, do aag operator\<\< inside the repo directory. Alloperator<< go to ostream from which they get redirected.

But@dcdillon just reminded me that I can't do this as a member function. So it was a null-op ;-)

Copy link

Contributor

nathan-russell commentedNov 27, 2016

I thinkfoo is failing becauseoperator double() is being called instead ofoperator<<. Changing the definition to this works for me:

// inline std::ostream &operator<<(std::ostream & s) const {//     s << this->format() << std::endl;//     return s;// }friend std::ostream&operator<<(std::ostream& os,const Date& d) {            os << d.format() << std::endl;return os;        }

Copy link

MemberAuthor

eddelbuettel commentedNov 27, 2016

Yes,@dcdillon and I chatted about that. I now have this working

  // end of public block  friend inline std::ostream &operator<<(std::ostream & s, const Date d);  // outside class    inline std::ostream &operator<<(std::ostream & os, const Date d) {        os << d.format();        return os;    }

and that worketh even ;-)

R> library(Rcpp)R> cppFunction('void foo(Date d) { Rcpp::Rcout << d << std::endl; } ')R> foo(Sys.Date())2016-11-27R>

eddelbuettel added2 commits

November 27, 2016 11:51

corrections to operator<<() for Date(time)

d41a48d

now it even works.  the amazement.

adding operator<< for Date(time)Vector

d85db8e

Copy link

MemberAuthor

eddelbuettel commentedNov 28, 2016

If anybody else would like to 'bless' (or even "review") it I'd feel better before merging my own PR :)

kevinushey reviewed

Nov 28, 2016

View reviewed changes

inst/include/Rcpp/date_datetime/Date.h Outdated

		char txt[32];
		structtm temp = m_tm;
		temp.tm_year -=baseYear();// adjust for fact that system has year rel. to 1900
		::strftime(txt,31, fmt, &temp);

Copy link

Contributor

kevinusheyNov 28, 2016

Choose a reason for hiding this comment

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

Should we check the return value of::strftime() here? What happens totxt if the user passes in afmt string not recognized by::strftime()?

Copy link

MemberAuthor

eddelbuettelNov 28, 2016

Choose a reason for hiding this comment

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

Thenstrftime() returns zero. I guess we could check and return an explicitstd::string(""). Would be cleaner.

inst/include/Rcpp/date_datetime/Date.h Outdated

		structtm temp = m_tm;
		temp.tm_year -=baseYear();// adjust for fact that system has year rel. to 1900
		::strftime(txt,31, fmt, &temp);
		returnstd::string(txt);

Copy link

Contributor

kevinusheyNov 28, 2016

Choose a reason for hiding this comment

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

Sincetxt isn't zero-initialized (it's a plain C array), it seems like it could be possible that no null byte exists at the end oftxt. Or does::strftime() write a null byte at the end? What if the output of::strftime() doesn't fit intxt?

Copy link

MemberAuthor

eddelbuettelNov 28, 2016

Choose a reason for hiding this comment

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

Per previous point andman strftime, by catching a return of zero we should catch that.

inst/include/Rcpp/date_datetime/Datetime.h Outdated

		char txtiso[64], txtsec[64];
		time_t t =static_cast<time_t>(std::floor(m_dt));
		structtm temp = *localtime(&t);// localtime, not gmtime
		::strftime(txtiso,63, fmt, &temp);

Copy link

Contributor

kevinusheyNov 28, 2016

Choose a reason for hiding this comment

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

Similar questions here re: zero-initialization, checking of return value?

Copy link

Contributor

kevinushey commentedNov 28, 2016

Some paranoid checks re: state of thechar array used in conjunction with::strftime() but otherwise LGTM.

explicit checks of return value in format

b264d8a

Copy link

MemberAuthor

eddelbuettel commentedNov 28, 2016

Thanks,@kevinushey . I extended bothformat() functions to be a wee bit more explicit on the return values.

Copy link

Contributor

kevinushey commentedNov 28, 2016

Okay, LGTM!

Copy link

MemberAuthor

eddelbuettel commentedNov 28, 2016

Thank you. I just added two (small) sets of unit tests which I'll in a minute and then I'll merge.

unit tests for Date(time) formatters

06fb890

eddelbuettel merged commit7a22d21 intomaster

Nov 28, 2016

eddelbuettel deleted the feature/format_dates branch

November 28, 2016 17:50

Labels

None yet

Movatterモバイル変換

Uh oh!

added format() methods for Date and Datetime#599

added format() methods for Date and Datetime#599

Uh oh!

Conversation

eddelbuettel commentedNov 27, 2016

Uh oh!

nathan-russell commentedNov 27, 2016

Uh oh!

eddelbuettel commentedNov 27, 2016

Uh oh!

eddelbuettel commentedNov 27, 2016

Uh oh!

eddelbuettel commentedNov 27, 2016

Uh oh!

coatlessNov 27, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddelbuettelNov 27, 2016

Choose a reason for hiding this comment

Uh oh!

nathan-russell commentedNov 27, 2016

Uh oh!

eddelbuettel commentedNov 27, 2016

Uh oh!

eddelbuettel commentedNov 28, 2016

Uh oh!

kevinusheyNov 28, 2016

Choose a reason for hiding this comment

Uh oh!

eddelbuettelNov 28, 2016

Choose a reason for hiding this comment

Uh oh!

kevinusheyNov 28, 2016

Choose a reason for hiding this comment

Uh oh!

eddelbuettelNov 28, 2016

Choose a reason for hiding this comment

Uh oh!

kevinusheyNov 28, 2016

Choose a reason for hiding this comment

Uh oh!

kevinushey commentedNov 28, 2016

Uh oh!

eddelbuettel commentedNov 28, 2016

Uh oh!

kevinushey commentedNov 28, 2016

Uh oh!

eddelbuettel commentedNov 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coatlessNov 27, 2016•
edited
Loading