This is an archive of the discontinued LLVM Phabricator instance.

[TimeProfiler] Emit real process ID and thread names
ClosedPublic

Authored by broadwaylamb on Apr 13 2020, 8:45 AM.

Diff Detail

Event Timeline

broadwaylamb created this revision.Apr 13 2020, 8:45 AM

Remove unrelated changes

Explicitly specify argument types in writeMetadataEvent lambda.

russell.gallop accepted this revision.Apr 14 2020, 7:15 AM

Would be nice if the test could check that the pid was being set to something but not sure how you could do that (beyond checking that it's a number).

Apart from that, LGTM.

This revision is now accepted and ready to land.Apr 14 2020, 7:15 AM
MaskRay requested changes to this revision.Apr 14 2020, 12:20 PM
MaskRay added inline comments.
llvm/lib/Support/TimeProfiler.cpp
76

You can define ThreadName as SmallString<0> to avoid this function.

231

The new line is not necessary

234

Delete {} for simple statements. In LLVM code, such {} is not common.

This revision now requires changes to proceed.Apr 14 2020, 12:20 PM
broadwaylamb marked 2 inline comments as done.Apr 14 2020, 12:31 PM
broadwaylamb added inline comments.
llvm/lib/Support/TimeProfiler.cpp
76

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?

234

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.

MaskRay added inline comments.Apr 14 2020, 1:10 PM
llvm/lib/Support/TimeProfiler.cpp
76

This is OK. Thanks

79

return Name.str();

234

They were added without enough scrutiny. There are many other simple statements in this file not surrounded by {}

MaskRay added inline comments.Apr 14 2020, 1:52 PM
llvm/lib/Support/TimeProfiler.cpp
234

See D78153

Fix coding style

broadwaylamb marked 4 inline comments as done.Apr 16 2020, 8:11 AM
MaskRay accepted this revision.Apr 16 2020, 10:08 AM
MaskRay added inline comments.
llvm/lib/Support/TimeProfiler.cpp
222

This is not great but I guess it is ok...

This revision is now accepted and ready to land.Apr 16 2020, 10:08 AM
broadwaylamb marked an inline comment as done.Apr 16 2020, 10:36 AM
broadwaylamb added inline comments.
llvm/lib/Support/TimeProfiler.cpp
222

What is?

This revision was automatically updated to reflect the committed changes.