[PATCH] D79678: [clangd] Add CSV export for trace metrics
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon May 11 03:43:21 PDT 2020
sammccall marked 6 inline comments as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:210
+ assert(!needsQuote(Metric.Name));
+ std::string QuotedLabel;
+ if (needsQuote(Label))
----------------
kadircet wrote:
> do we ever expect to have any of `\r \n , "` in a label? I think it would be OK to just assert notNeedsQuote on Label.
>
> Up to you though.
I don't think it's terribly likely but I can certainly imagine:
- using a filename as a label, and some people have weird filenames
- wanting a compound label, and choosing comma as a separator
I was torn between:
- doing the escaping (seems a little silly)
- asserting (a bit of a time bomb in cases above)
- just ignoring this and outputting garbage
Chose A because it's pretty short, and I wrote a test :-)
================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:251
+ using namespace std::chrono;
+ return MicrosT0SinceEpoch +
+ duration<double, std::micro>(steady_clock::now() - SteadyT0).count();
----------------
kadircet wrote:
> what are the benefits for making these absolute apart from being able to merge streams coming from different runs?
> I am not sure if we'll ever merge data from multiple users, or even multiple runs from the same user.
>
> This would help decrease output size only by a couple percents though, so not that important. Just wondering if there are any other reasons.
Yeah no good reason I think. Removed.
Was thinking about converting into walltimes in analysis but it seems unlikely.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79678/new/
https://reviews.llvm.org/D79678
More information about the cfe-commits
mailing list