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

Q: any reason for which date:format(fmt, local_time,...) methods take 'timepoint' by value? #82

Closed
acolomitchi opened this issue Sep 13, 2016 · 8 comments

Comments

@acolomitchi
Copy link
Contributor

I looked into the code and, half-heartedly, I saw/accepted the reason of passing the std::string fmt by value.

But why the tp (timepoint) parameter is by value and not by const local_time&?

Assuming one uses local_time vars as data-members of other classes/struct, most of the time a format(fmt, tp) call will originate in cases in which the parent struct is subject to 'stringification' - e.g. operator << (ostream&, const parent_struct& toDump) - in which cases, the parent struct will be passed 'by const reference'.
Yes, of course, one can create a local copy for that local_time data member (with optimisation level high enough, it'll be an operation as cheap as creating and init-ing a long), but it's... I don't know... say, annoyingly opposite with 'output ops traditions'?

@HowardHinnant
Copy link
Owner

HowardHinnant commented Sep 13, 2016

The only rationale that I used was that a chrono::time_point is typically the same size as a scalar (more often 64 bit but sometimes 32 bit), and that the compiler would probably pass it by value in a register just as easily.

However I never measured (before now).

I just did an experiment inspecting the optimized assembly of this test:

std::string
test(std::chrono::system_clock::time_point tp)
{
    return date::format("%F %T", tp);
}

There were differences, but the differences are likely in the noise level. Some operations were moved from one place to another. Overall, the line count on the assembly listing was 0.29% smaller for the const& test.

I can't really get excited about one way vs the other. However if you prefer the const& version and no one else prefers the by-value version, I'll make it const&.

@acolomitchi
Copy link
Contributor Author

Thanks - all things being equal, I do prefer the const local_time& format for reasons pertaining to the constness of parameters involved in string-output operations.

I can get around with my own overrides/wrappers which take a const &, make a copy and calls into the target function - 5-10 extra lines of code for the entire project - so, low priority, no hurry and no probs if you close this with WONTFIX.
As I said, it's not a matter of performance/overhead, but rather a matter of lines of code one needs to write to achieve the needed functionality (for some times now it got stuck in my head this weird idea of 'while an executable is an assets, to source code required to get it is a liability'. And I'm afraid I won't be able to get help from any shrink on the matter ;) )

@acolomitchi
Copy link
Contributor Author

acolomitchi commented Sep 13, 2016

Ah, on the same line... would you consider++ doubling the number of format functions with ostream& format(ostream& os, ...)-signature methods? After all, all the format functions land into

date::detail::format(....) {
    // heaps of work and then at the end...
    f.put(os, os, os.fill(), &tm, fmt.data(), fmt.data() + fmt.size());
   // -----^-- already an ostream
    return os.str();
}

++ ;) do they call this "cost externalization"?
Seriously speaking, if you accept the request, it'd be Ok for me to do it and raise a PR.

@HowardHinnant
Copy link
Owner

I'm not crazy about the idea. But I'll give it some thought.

@HowardHinnant
Copy link
Owner

The pass by const& issue is addressed in 86446a9

I'm leaving this open for now as I'm still contemplating your suggestions on format streaming directly to a ostream.

@acolomitchi
Copy link
Contributor Author

Thanks.

The argument in favour of the second is a matter of performance and performance only - the second form breaks the 'fluency' of the l-shift operator

  std::cout << "Date of birth: " << date::format(fmtstr, tp) << std::endl
            << "Sex: yes, please" << std::endl
  ;

vs

  std::cout << "Date of birth:";
  date::format(std::cout,fmtstr, tp);
  std::cout << std::endl
            << "Sex: yes, please" << std::endl
  ;

But I'd choose the second when performance is required and code fluency is not an issue (e.g. remote services with json interfaces)

@HowardHinnant
Copy link
Owner

HowardHinnant commented Sep 14, 2016

The 2nd form also doesn't respect width and adjustfield formatters. And even if I create the 2nd form, it will still have to create ostringstream internally for %z and for %S and %T when the precision is finer than seconds (somewhat nullifying the performance argument).

That being said, your request also induced me to take another look at parse. I have an experiment running now that accepts this syntax:

std::istringstream infile{"2013-06-13 11:00:45 2013-06-14 09:03:23"};
date::sys_seconds tp1, tp2;
infile >> date::parse("%F %T", tp1) >> date::parse("%F %T", tp2);

which aligns with the current use of format better.

@HowardHinnant
Copy link
Owner

Closing for now as the primary issue has been addressed. At this time I don't anticipate creating a format which takes an ostream and returns void. However feel free to open a new issue on this subject if you desire. I have now committed additions to parse so that it can be used as described per my comment above. This alternative syntax is not significantly different performance wise. It is just an alternative syntax that I like.

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

No branches or pull requests

2 participants