sharkdp / dbg-macro

A dbg(…) macro for C++
MIT License
2.97k stars 257 forks source link

added timestamp #91

Closed MrHate closed 4 years ago

MrHate commented 4 years ago

90

sharkdp commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

MrHate commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

:+1:

MrHate commented 4 years ago

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?

MrHate commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Looks like that works on both platforms :fireworks:

MrHate commented 4 years ago

I'm glad it works, thanks again David!