sammccall accepted this revision. sammccall added a comment. Thanks!
In D78429#2013635 <https://reviews.llvm.org/D78429#2013635>, @kadircet wrote: > Also while writing the test for rename I've noticed we were actually counting > renamed files rather than occurrences, had to put a loop that would go over > each file, PTAL. Oh, I'd forgotten this. This was actually deliberate IIRC, I think the idea was much of the "cost" of a large rename grows with files rather than occurrences, so the limit of 50 applies to files. I'd probably leave as-is and rename the metric to rename_files, but up to you. ================ Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:159 + llvm::consumeError(Client.call(MethodName, {}).take().takeError()); + EXPECT_THAT(Tracer.take("lsp_latency", MethodName), Not(testing::IsEmpty())); +} ---------------- nit: ElementsAre(_) or SizeIs(1)? ================ Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:522 + EXPECT_THAT(Tracer.take("ast_access_diag", "hit"), IsEmpty()); + EXPECT_THAT(Tracer.take("ast_access_diag", "miss"), IsEmpty()); ---------------- SizeIs(0)/SizeIs(1) might be more expressive here ================ Comment at: clang-tools-extra/clangd/unittests/TestTracer.h:35 + /// Returns recorded measurements for \p Metric and clears them. + std::vector<double> take(std::string Metric, std::string Label = ""); + ---------------- ah, this should probably be takeMetric or so, because we might extend this class to cover non-metric tracing stuff. (a slight misnomer, but "take measurement" collides with an english idiom and is also a bit unwieldy) ================ Comment at: clang-tools-extra/clangd/unittests/support/TestTracer.cpp:18 + llvm::StringRef Label) { + Measurements[Metric.Name][Label].push_back(Value); +} ---------------- this needs a lock, the interface is documented as threadsafe. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78429/new/ https://reviews.llvm.org/D78429 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
