[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi added a comment. @jkorous Can you lemme know if you're happy with https://reviews.llvm.org/D65829. @gribozavr lemme also know any other feedback in D65829 , as this diff here is already closed. Repository: rL LLVM CHANGES SINCE LAST ACTION

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi marked 2 inline comments as done. plotfi added inline comments. Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287 /*waitForInitialSync=*/true); + if (!DW) return; plotfi wrote: > gribozavr wrote: > > plotfi wrote: > >

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi marked 2 inline comments as done. plotfi added inline comments. Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287 /*waitForInitialSync=*/true); + if (!DW) return; gribozavr wrote: > plotfi wrote: > > gribozavr wrote:

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

gribozavr added inline comments. Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287 /*waitForInitialSync=*/true); + if (!DW) return; plotfi wrote: > gribozavr wrote: > > plotfi wrote: > > > gribozavr wrote: > > > > Why? This

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi marked 2 inline comments as done. plotfi added inline comments. Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102 - /// Returns nullptr if \param Path doesn't exist or isn't a directory. - /// Returns nullptr if OS kernel API told us we can't

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi marked an inline comment as done. plotfi added inline comments. Comment at: cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:329 + assert(!Path.empty() && "Path.empty()"); const int InotifyFD = inotify_init1(IN_CLOEXEC); @gribozavr

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi marked an inline comment as done. plotfi added inline comments. Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287 /*waitForInitialSync=*/true); + if (!DW) return; gribozavr wrote: > plotfi wrote: > > gribozavr wrote:

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi marked an inline comment as done. plotfi added inline comments. Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102 - /// Returns nullptr if \param Path doesn't exist or isn't a directory. - /// Returns nullptr if OS kernel API told us we can't

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

gribozavr added inline comments. Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102 - /// Returns nullptr if \param Path doesn't exist or isn't a directory. - /// Returns nullptr if OS kernel API told us we can't start watching. In such - /// case

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

jkorous added inline comments. Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102 - /// Returns nullptr if \param Path doesn't exist or isn't a directory. - /// Returns nullptr if OS kernel API told us we can't start watching. In such - /// case

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi marked 6 inline comments as done. plotfi added inline comments. Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102 - /// Returns nullptr if \param Path doesn't exist or isn't a directory. - /// Returns nullptr if OS kernel API told us we can't

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi marked an inline comment as done. plotfi added inline comments. Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283 /*waitForInitialSync=*/true); + if (!DW) return; gribozavr wrote: > plotfi wrote: > > jkorous wrote: > > >

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

gribozavr added inline comments. Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283 /*waitForInitialSync=*/true); + if (!DW) return; plotfi wrote: > jkorous wrote: > > jkorous wrote: > > > IIUC this is silently dropping errors.

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

gribozavr added inline comments. Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102 - /// Returns nullptr if \param Path doesn't exist or isn't a directory. - /// Returns nullptr if OS kernel API told us we can't start watching. In such - /// case

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL367979: [clang][DirectoryWatcher] Adding llvm::Expected error handling to create. (authored by zer0, committed by ).

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi updated this revision to Diff 213520. plotfi added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. add another assert Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65704/new/ https://reviews.llvm.org/D65704

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

compnerd added inline comments. Comment at: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:218 +return llvm::make_error( +std::string("Path.empty() error: ") + strerror(errno), +llvm::inconvertibleErrorCode()); compnerd wrote: >

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi updated this revision to Diff 213515. plotfi added a comment. Improve comments, change Path.empty() errors for asserts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65704/new/ https://reviews.llvm.org/D65704 Files:

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi marked 3 inline comments as done. plotfi added inline comments. Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:102 /// Returns nullptr if \param Path doesn't exist or isn't a directory. /// Returns nullptr if OS kernel API told us we can't

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

jkorous added a comment. Please just update the comment, otherwise LGTM. Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283 /*waitForInitialSync=*/true); + if (!DW) return; jkorous wrote: > IIUC this is silently dropping

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

jkorous added a comment. Thanks for the patch! Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:102 /// Returns nullptr if \param Path doesn't exist or isn't a directory. /// Returns nullptr if OS kernel API told us we can't start watching. In such

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi updated this revision to Diff 213221. plotfi added a comment. Fix a linux typo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65704/new/ https://reviews.llvm.org/D65704 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi added a comment. In D65704#1613688 , @compnerd wrote: > Rather than silently ignoring tests when the DirectoryWatcher isn't created, > can you please print an error message and exit with an error code to indicate > the test failed?

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi updated this revision to Diff 213218. plotfi added a comment. More cleanup Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65704/new/ https://reviews.llvm.org/D65704 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

compnerd added a comment. BTW, I think that we should add a test case to ensure that we see the error in the case that the inotify fds are exhausted. We should be able to create a process and set the limit for that process to 0/1 and use that to trigger the failure. Repository: rG LLVM

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

compnerd added a comment. Rather than silently ignoring tests when the DirectoryWatcher isn't created, can you please print an error message and exit with an error code to indicate the test failed? Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:331 +

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi updated this revision to Diff 213209. plotfi added a comment. Cleanup some stuff Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65704/new/ https://reviews.llvm.org/D65704 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi marked an inline comment as done. plotfi added inline comments. Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:32 #include +#include This include should be removed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

plotfi created this revision. plotfi added reviewers: jkorous, compnerd. Herald added subscribers: cfe-commits, dexonsmith. Herald added a project: clang. There are cases where the DirectoryWatcherTests just hang in a deadlock when there are inotify limits hit, with no error reporting. This