Skip to content

Conversation

eddelbuettel
Copy link
Member

also rolled dev version in config.h

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

rolled dev version in config.h
@nathan-russell
Copy link
Contributor

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 to flush is not desirable.

@eddelbuettel
Copy link
Member Author

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?).

@eddelbuettel
Copy link
Member Author

Also annoying:

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

I'd love for foo() to do what bar() does.

@eddelbuettel
Copy link
Member Author

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

return std::string(txtsec);
}

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

@coatless coatless Nov 27, 2016

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 or Rcpp::Rstreambuf instead of std::ostream?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, do a ag operator\<\< inside the repo directory. All operator<< 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 ;-)

@nathan-russell
Copy link
Contributor

I think foo is failing because operator double() is being called instead of operator<<. 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;
        }

@eddelbuettel
Copy link
Member Author

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-27
R>

@eddelbuettel
Copy link
Member Author

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

char txt[32];
struct tm 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

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 to txt if the user passes in a fmt string not recognized by ::strftime()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then strftime() returns zero. I guess we could check and return an explicit std::string(""). Would be cleaner.

struct tm temp = m_tm;
temp.tm_year -= baseYear(); // adjust for fact that system has year rel. to 1900
::strftime(txt, 31, fmt, &temp);
return std::string(txt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since txt 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 of txt. Or does ::strftime() write a null byte at the end? What if the output of ::strftime() doesn't fit in txt?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

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?

@kevinushey
Copy link
Contributor

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

@eddelbuettel
Copy link
Member Author

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

@kevinushey
Copy link
Contributor

Okay, LGTM!

@eddelbuettel
Copy link
Member Author

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

@eddelbuettel eddelbuettel merged commit 7a22d21 into master Nov 28, 2016
@eddelbuettel eddelbuettel deleted the feature/format_dates branch November 28, 2016 17:50
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.

4 participants