jkorous marked 2 inline comments as done. jkorous added a comment. Thanks for taking a look @gribozavr!
I briefly scanned the rest of your comments and I agree with you (or don't really have a strong opinion) in all cases. I'll work on that today. ================ Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:136 + + const int PollResult = poll(&PollReq, 1, TimeoutMs); + // There are inotify events waiting to be read! ---------------- gribozavr wrote: > What is the role of the timeout and why does it need to be so small? The whole idea is that we can't block on `read()` if we ever want to stop watching the directory, release resources (file descriptors, threads) and correctly destruct the DirectoryWatcher instance either - because of a bug in some other thread in the implementation - or asynchronous client action (e. g. destructor being called) in main application thread The timeout adds latency in those scenarios. ================ Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:131 + // the async event handling picks them up. Can make this test flaky. + std::this_thread::sleep_for(std::chrono::milliseconds(100)); // 0.1s +} ---------------- gribozavr wrote: > I'm certain this sleep will be flaky on heavily-loaded CI machines. If you > are going to leave it as a sleep, please make it 1s. But is there really no > better way? That was exactly my thinking! Honestly, I don't know - I wasn't able to come up with any reasonably simple, deterministic approach even on a single platform :( I eventually picked 0.1s as a tradeoff between slowing the test for everyone and getting less false positives. The problem as I understand it is that we're making changes and monitoring them asynchronously with no guarantee from the kernel API (true for FSEvents, not 100% about inotify) about when (if) we receive notifications. If you have any idea for robust testing approach I'd be totally happy to use it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
