! added option to specify the format to display timings #52
Conversation
! defaults to nanoseconds ! supports miliseconds and seconds
src/AnalysisExporter/FileCompilations/FileCompilationsExporter.cpp
Outdated
Show resolved
Hide resolved
src/AnalysisExporter/TemplateInstantiations/TemplateInstantiationsExporter.cpp
Outdated
Show resolved
Hide resolved
src/BuildAnalyzer/Analyzers/FileCompilations/FileCompilationsAnalyzer.cpp
Show resolved
Hide resolved
| if (timingDisplay == "ms") | ||
| { | ||
| analysisOptions.TimeDisplay = BuildAnalyzer::TimeDisplayEnum::Mili; | ||
| } | ||
| else if (timingDisplay == "s") | ||
| { | ||
| analysisOptions.TimeDisplay = BuildAnalyzer::TimeDisplayEnum::Sec; | ||
| } |
There was a problem hiding this comment.
Can we show a message when we get other option saying we'll use the default representation (and which one)?
There was a problem hiding this comment.
Can you please add the case where user passes ns as the time unit?
It's currently displaying the Unrecognized time display value although ns is a valid one.
Thank you!
src/ConsoleMain/Main.cpp
Outdated
| ("out_template_instantiations", "Path to output template instantiations data", cxxopts::value(outputPathTemplateInstantiations)); | ||
| ("out_template_instantiations", "Path to output template instantiations data", cxxopts::value(outputPathTemplateInstantiations)) | ||
| //timing display | ||
| ("timing", "defaults to ns, can use ms(miliseconds) or s(seconds)", cxxopts::value(timingDisplay)) |
There was a problem hiding this comment.
Although I like timing, it doesn't feel related to the time unit. How about giving it a name that makes it crystal clear?
Also, some spell-check and tweaks:
- Description should start in a capital letter.
miliseconds->milliseconds.- Consider using
ns(nanoseconds), as you've used that format with the other units.
Finally, as mentioned in previous comment, if you consider making s(seconds) the default, please update the description.
There was a problem hiding this comment.
Naming is hard :(
display_time_unit ?
the rest are fixed.
There was a problem hiding this comment.
Yeah, display_time_unit works!
Please go with it :)
! fixed spelling
|
Thank you very much for taking the time to review my work and for your amazing work. |
|
I've been running some tests and found an issue I had overlooked: any of the time units we select will only display the value as an integer. What I'd love it to be is:
I'm okay with having:
What do you think? |
3 participants
! defaults to nanoseconds
! supports miliseconds and seconds