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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
