[PATCH] D78027: [TimeProfiler] Emit real process ID and thread names

Fangrui Song via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 14 13:31:07 PDT 2020


MaskRay added inline comments.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:77
 
+static std::string getThreadName() {
+  SmallString<64> Name;
----------------
broadwaylamb wrote:
> MaskRay wrote:
> > You can define ThreadName as `SmallString<0>` to avoid this function.
> Then the `ThreadName` field will lose `const`-ness, and I will also need to call `llvm::get_thread_name` in the constructor body, not the member initialization list. Would that be OK?
This is OK. Thanks


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:80
+  llvm::get_thread_name(Name);
+  return std::string(std::move(Name));
+}
----------------
`return Name.str();`


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:249
+
+    for (const auto &TTP : ThreadTimeTraceProfilerInstances) {
+      writeMetadataEvent("thread_name", TTP->Tid, TTP->ThreadName);
----------------
broadwaylamb wrote:
> MaskRay wrote:
> > Delete `{}` for simple statements. In LLVM code, such `{}` is not common.
> The code above uses `{}`.
> 
> According to http://llvm.org/docs/CodingStandards.html:
> 
> >If you are extending, enhancing, or bug fixing already implemented code, use the style that is already being used so that the source is uniform and easy to follow.
They were added without enough scrutiny. There are many other simple statements in this file not surrounded by `{}`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78027/new/

https://reviews.llvm.org/D78027





More information about the cfe-commits mailing list