added timestamp #91

MrHate · 2020-02-23T06:14:06Z

sharkdp · 2020-02-25T19:54:38Z

Thank you for your feedback and for your contribution!

I would like to avoid adding more (compile time) configuration options to dbg-macro because the number of scenarios that have to be tested increases a lot (exponentially).

We could think about adding this by default. The problem I see here is that the current format (HH:MM:SS) is only going to be useful for scenarios where the user program runs for more than one second. If that is not the case, the user would probably like to see a (much) higher resolution in the time output. Going down to the microsecond level would lead to a HH:MM:SS.SSSSSS format that takes away 15 characters (plus padding) of the valuable horizontal screen width.

We could also think about implementing this in a different way where we add an easy option to print the time with dbg(…). Maybe something like

dbg(dbg::time())

or

dbg(dbg::time)

What do you think? What is your use case?

MrHate · 2020-02-26T02:19:24Z

Your thought is better. This is just a proposal for you guys. In fact the version with microsecond can be expected to be more useful, but I did also consider about the trade off between microsecond and characters, so I just came out with the configuration. I'd like to see a better version with your solution.

sharkdp · 2020-02-27T21:18:52Z

I'd like to see a better version with your solution.

With which one? The dbg(dbg::time()) suggestion?

If yes, would you be interested to try an implementation? Otherwise, should we close this?

MrHate · 2020-02-28T01:32:52Z

Sorry I did misunderstand your meaning.
I will try to implement the the dbg(dbg::time()) version.

MrHate · 2020-02-28T04:58:04Z

I've implemented dbg(dbg::time()) version here, now it works like this:

[..p/dbg/tests/demo.cpp:140 (main)] 13:37:03.745746

How do you think about it?

And I still haven't a good idea on how to test this function.

DerekCresswell · 2020-03-03T03:56:26Z

Just a thought what if we had two functionalities :
dbg::time() prints out a time stamp

Then similar to hex we can do :
dbg(dbg::time(x)) to print x with a timestamp as well.

If that sounds cool I'd be willing to give it a go.

sharkdp · 2020-03-03T07:14:04Z

Then similar to hex we can do :
dbg(dbg::time(x)) to print x with a timestamp as well.

Hm. I think that's not quite the same as dbg::hex(…). I'd rather stick with an argument-less dbg::time() and work on #2 in order to support things like

dbg(dbg::time(), x)

If that sounds cool I'd be willing to give it a go.

👍

MrHate · 2020-03-03T10:37:10Z

I combined my commits into one, so my changes can be reviewed more clearly :)
So how about my pr now, do I need revision or something else?

sharkdp · 2020-03-06T21:51:43Z

dbg.h

+namespace timestamp {
+
+// Provide a timestamp at the precision of microsecond.
+// Not exactly the current microsecond yet, still a useful reference.


Well, one option would be to make timestamp_t an empty struct and only determine the time when it's actually printed.

This option sounds so genius! I cannot agree anymore. From your suggestion I've got another idea, how about just using struct time {};?

sharkdp · 2020-03-06T21:52:35Z

dbg.h

+// Provide a timestamp at the precision of microsecond.
+// Not exactly the current microsecond yet, still a useful reference.
+
+struct timestamp_t {


I suggest we remove the namespace timestamp and simply name this type timestamp (it's still in the dbg namespace).

I just thought I could use this namespace to distinguish the relevant code for the timestamp, but I can remove it if unnecessary.

sharkdp · 2020-03-06T21:54:08Z

dbg.h

@@ -626,6 +649,19 @@ class DebugOutput {
    return std::forward<T>(value);
  }

+  timestamp::timestamp_t&& print(const std::string& type, timestamp::timestamp_t value) const {


Instead of repeating the whole printing logic here, we could write an overload for pretty_print.

A specialized pretty_print works neat and elegant indeed and this is how this project works, but this would prevent me from using ANSI_XXXs. I just think printing timestamp with color might be better, but I can to forget about it if unnecessary.

sharkdp · 2020-03-06T21:55:03Z

tests/demo.cpp

@@ -7,6 +7,8 @@
 #include <string>
 #include <tuple>
 #include <vector>
+#include <ctime>
+#include <chrono>


These includes are not necessary, right?

Sure, I did forget about them after testing.

sharkdp · 2020-03-09T18:34:59Z

dbg.h

+template <>
+inline bool pretty_print(std::ostream& stream, const time&) {
+  const auto ms =  std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::high_resolution_clock::now().time_since_epoch()).count() % 1000000;
+  const auto hms = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now());


I think we should use the same timestamp here as for the ms computation. Otherwise, this could lead to inconsistencies.

I'll make the necessary changes.

sharkdp · 2020-03-09T18:41:02Z

dbg.h

@@ -453,6 +457,16 @@ inline bool pretty_print(std::ostream& stream, const std::tuple<>&) {
  return true;
 }

+template <>
+inline bool pretty_print(std::ostream& stream, const time&) {
+  const auto ms =  std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::high_resolution_clock::now().time_since_epoch()).count() % 1000000;


ms is slightly confusing, as it usually describes milliseconds. I'll rename it to us.

sharkdp · 2020-03-09T18:41:12Z

dbg.h

+  const auto hms = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now());
+  const std::tm* tm = std::localtime(&hms);
+  stream << std::put_time(tm, "%H:%M:%S") << '.'
+         << ms ;


This will print a wrong value if ms has a value smaller than 100000, because the leading zero(s) are missing. If ms = 12345, it should print HH:MM:SS.012345

sharkdp

Thank you very much!

MrHate · 2020-03-09T19:00:47Z

I should say thanks. Cause I never expect things could be this complicated, and thus I've learned a lot from this.
But another question is, there is a error here that high_resolution_clock cannot be converted into system_clock, and there is no member named to_time_t in high_resolution_clock. My idea is just make the microsecond as a reference but not exactly the current microsecond cause it cannot be such precise itself, so maybe it's not a bad thing to divide the microsecond part out.

sharkdp · 2020-03-09T19:02:23Z

But another question is, there is a error here that high_resolution_clock cannot be converted into system_clock, and there is no member named to_time_t in high_resolution_clock.

Yeah, that happened on Windows because apparently high_resolution_clock refers to steady_clock there.

I have switched to system_clock. Let's see if that works.

sharkdp · 2020-03-09T19:05:40Z

Looks like that works on both platforms 🎆

MrHate · 2020-03-09T19:08:33Z

I'm glad it works, thanks again David!

sharkdp reviewed Mar 6, 2020

View reviewed changes

added timestamp

ee894ea

sharkdp reviewed Mar 9, 2020

View reviewed changes

Single timestamp and some code formatting

da5f4d3

sharkdp reviewed Mar 9, 2020

View reviewed changes

sharkdp added 3 commits March 9, 2020 19:43

Make sure that leading zeros are printed in us part

0628542

Different format for the prefix

ef8dae8

Wording in README

2cf0d0b

sharkdp approved these changes Mar 9, 2020

View reviewed changes

Use system_clock everywhere for Windows

1444836

sharkdp merged commit 1a1b798 into sharkdp:master Mar 9, 2020

sharkdp mentioned this pull request Mar 9, 2020

Provide an optional timestamp? #90

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added timestamp #91

added timestamp #91

MrHate commented Feb 23, 2020

sharkdp commented Feb 25, 2020

MrHate commented Feb 26, 2020 •

edited

sharkdp commented Feb 27, 2020 •

edited

MrHate commented Feb 28, 2020

MrHate commented Feb 28, 2020 •

edited

DerekCresswell commented Mar 3, 2020 •

edited

sharkdp commented Mar 3, 2020

MrHate commented Mar 3, 2020

sharkdp Mar 6, 2020

MrHate Mar 7, 2020

sharkdp Mar 6, 2020

MrHate Mar 7, 2020

sharkdp Mar 6, 2020

MrHate Mar 7, 2020

sharkdp Mar 6, 2020

MrHate Mar 7, 2020

sharkdp Mar 9, 2020

sharkdp Mar 9, 2020

sharkdp Mar 9, 2020

sharkdp left a comment

MrHate commented Mar 9, 2020

sharkdp commented Mar 9, 2020

sharkdp commented Mar 9, 2020

MrHate commented Mar 9, 2020

added timestamp #91

added timestamp #91

Conversation

MrHate commented Feb 23, 2020

sharkdp commented Feb 25, 2020

MrHate commented Feb 26, 2020 • edited

sharkdp commented Feb 27, 2020 • edited

MrHate commented Feb 28, 2020

MrHate commented Feb 28, 2020 • edited

DerekCresswell commented Mar 3, 2020 • edited

sharkdp commented Mar 3, 2020

MrHate commented Mar 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp left a comment

Choose a reason for hiding this comment

MrHate commented Mar 9, 2020

sharkdp commented Mar 9, 2020

sharkdp commented Mar 9, 2020

MrHate commented Mar 9, 2020

MrHate commented Feb 26, 2020 •

edited

sharkdp commented Feb 27, 2020 •

edited

MrHate commented Feb 28, 2020 •

edited

DerekCresswell commented Mar 3, 2020 •

edited