Signal watcher functionality with test #597
Conversation
vertexodessa
force-pushed
the
SignalWatcher
branch
6 times, most recently
from
d3a62e5 to
5ed87d2
Compare
vertexodessa
changed the title
vertexodessa
force-pushed
the
SignalWatcher
branch
3 times, most recently
from
7bc5f48 to
be3a26e
Compare
|
@stellaeof hi Stella, could you please review.. |
|
Hi Ihor, I'm pretty busy and on mobile so will be brief. Thanks for the
patch, but I would really rather not implement such a thing as a set of
macros.
Further, I don't really feel like this needs to be abstracted in a platform
neutral way since it is pretty tied to position/unix-like (ie we run this
on at least one posix platform where this would not be usable).
Wdyt about adding a posix_helpers.h/cc and putting it there? If in its own
area, there's no need to extend the compat layer. Also, can you factor this
more as a regular c++ api vs macros?
If we had a separate helper lib, it could be enabled via a makefile flag
that wouldn't include it for Windows and or other nutty platforms.
Thanks!
…On May 1, 2017 3:21 PM, "Ihor Ivlev" ***@***.***> wrote:
@stellaeof <https://github.com/stellaeof> hi Stella, could you please
review..
I had to steal std::thread, std::unique_lock and std::condition to pthread
platform implementation, not sure how critical is that, considering the
fact there are other std usages in pthread.
Maybe we'll have to implement them using pthread primitives as a
refactoring step later if that works?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#597 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAeKgF3UPJfNVMbRCwx-_Hsa51fJA1a9ks5r1lr6gaJpZM4NM5GQ>
.
|
vertexodessa
force-pushed
the
SignalWatcher
branch
from
be3a26e to
ef40415
Compare
|
@stellaeof hi Stella, thanks for the review, I've updated the pull request according to your suggestions and added the test. |
vertexodessa
force-pushed
the
SignalWatcher
branch
from
ef40415 to
e9bcba4
Compare
There was a problem hiding this comment.
Sorry this took so long - I was traveling. I've noted a number of nits where this code doesn't match the style guide the rest was written to, but I don't want to hold up your contribution. Feel free to submit and I can cleanup later (or there is a script in there to run clang-format, which I think will fix most of it).
The biggest naming thing is that functions are NamedLikeThis and variables are named_like_this.
bindings/cpp/posix_utils.cc
Outdated
| return; | ||
| } | ||
|
|
||
| static bool onlyOnce = false; |
There was a problem hiding this comment.
This is still a bit racy. I typically see a std::atomic used for this (with a condition on if (only_once.exchange(true))).
bindings/cpp/posix_utils_test.cc
Outdated
|
|
||
| #include "gtest/gtest.h" | ||
|
|
||
| char const* kSignalWatcherFilename = "./tmptstsignal_watcher.wtf-trace"; |
There was a problem hiding this comment.
Prefer: const char kSignalWatcherFilename[]
bindings/cpp/posix_utils_test.cc
Outdated
|
|
||
| TEST_F(PosixUtilsTest, SignalWatcher) { | ||
| unlink(kSignalWatcherFilename); | ||
| // watch SIGALRM |
bindings/cpp/posix_utils_test.cc
Outdated
| usleep(5000 * second); | ||
|
|
||
| alarm(0); | ||
| // allow the thread to finish it's work |
bindings/cpp/posix_utils_test.cc
Outdated
| EXPECT_TRUE(stat(kSignalWatcherFilename, &buffer) == 0); | ||
| EXPECT_TRUE(buffer.st_size > 0); | ||
| } | ||
| } //namespace wtf |
There was a problem hiding this comment.
Space between comment and "namespace"
vertexodessa
force-pushed
the
SignalWatcher
branch
from
e9bcba4 to
ae0f7db
Compare
vertexodessa
force-pushed
the
SignalWatcher
branch
from
ae0f7db to
790dde7
Compare
|
@stellaeof Stella, thanks a lot for the review. I updated the code-review according to your comments. |
The idea is to have simple macros which will allow to dump the current profiling result to a file by issuing "kill" shell command.