anton-afanasyev marked 7 inline comments as done. anton-afanasyev added a subscriber: aras-p. anton-afanasyev added a comment.
In D60663#1465721 <https://reviews.llvm.org/D60663#1465721>, @lebedev.ri wrote: > Looks good ignoring the json bits. > > Re license: > > > https://reviews.llvm.org/D58675 > > This is the first part of time tracing system, I have splitted them cause > > this part is mostly written by Aras Pranckevicius except of several minor > > fixes concerning formatting. > > So i can't and won't claim any legal knowledge, but it maybe would be good > for him to at least comment here, that he is ok with this? Oh, sure! @aras-p -- I have changed license header to fit it with current LLVM Relicensing state. Are you ok with this? (see https://reviews.llvm.org/D60663#change-jr7Lagn5WNFy) ================ Comment at: llvm/lib/Support/TimeProfiler.cpp:20 #include <string> #include <unordered_map> #include <vector> ---------------- lebedev.ri wrote: > Unused header? Yes, thanks. ================ Comment at: llvm/lib/Support/TimeProfiler.cpp:47-48 void begin(std::string Name, llvm::function_ref<std::string()> Detail) { - Entry E = {steady_clock::now(), {}, Name, Detail()}; + Entry E = {steady_clock::now(), {}, std::move(Name), Detail()}; Stack.push_back(std::move(E)); } ---------------- lebedev.ri wrote: > Does > ``` > Stack.emplace_back(steady_clock::now(), {}, std::move(Name), Detail()); > ``` > not work? > (also, the `std::string` returned from `Detail` function invocation is moved?) Ok, changed. ================ Comment at: llvm/lib/Support/TimeProfiler.cpp:57 // Only include sections longer than 500us. if (duration_cast<microseconds>(E.Duration).count() > 500) Entries.emplace_back(E); ---------------- lebedev.ri wrote: > I feel like this should be > ``` > static cl::opt<unsigned> TimeProfileGranularity( > "time-profile-granularity", > cl::desc("<fixme>"), > cl::init(500)); > ``` > ? I planned to change this later together with unit tests (cause they need small time granularity), but can fix it now. Changed, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60663/new/ https://reviews.llvm.org/D60663 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
