kadircet accepted this revision.
kadircet marked 2 inline comments as done.
kadircet added a comment.
This revision is now accepted and ready to land.
Thanks, LGTM!
================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:210
+ assert(!needsQuote(Metric.Name));
+ std::string QuotedLabel;
+ if (needsQuote(Label))
----------------
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.
================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:214
+ uint64_t Micros = timestamp();
+ std::string Scratch;
+ Out << llvm::formatv("{0},{1},{2},{3:e},{4}.{5:3}\r\n",
----------------
unused var
================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:215
+ std::string Scratch;
+ Out << llvm::formatv("{0},{1},{2},{3:e},{4}.{5:3}\r\n",
+ typeName(Metric.Type), Metric.Name, Label, Value,
----------------
acquire `Mu` before printing ?
================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:221
+private:
+ llvm::StringRef typeName(Metric::MetricType T) {
+ switch (T) {
----------------
why not use integers instead?
================
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();
----------------
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.
================
Comment at: clang-tools-extra/clangd/unittests/support/TraceTests.cpp:183
+ llvm::SmallVector<llvm::StringRef, 4> Lines;
+ llvm::StringRef(Output).split(Lines, "\r\n");
+ EXPECT_THAT(Lines, ElementsAre(_, StartsWith(R"(d,dist,",",1)"),
----------------
nit: maybe have a `getLines()` in the fixture.
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
