gribozavr added inline comments.
================ Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:323 + /*waitForInitialSync=*/false); + // llvm::Expected will throw an error if DW is an Error. + if (!DW) ---------------- plotfi wrote: > gribozavr wrote: > > plotfi wrote: > > > gribozavr wrote: > > > > Call `llvm::cantFail` and there will be no need to explain anything. > > > I explained this in the other patch: > > > > > > cantFail is arguably worse. I want this test to tell me the cause of the > > > crash (in this case, exceeding the inotify limit). What I want to happen > > > is: > > > > > > ``` > > > Note: Google Test filter = DirectoryWatcherTest.AddFiles > > > [==========] Running 1 test from 1 test case. > > > [----------] Global test environment set-up. > > > [----------] 1 test from DirectoryWatcherTest > > > [ RUN ] DirectoryWatcherTest.AddFiles > > > Expected<T> must be checked before access or destruction. > > > Unchecked Expected<T> contained error: > > > inotify_init1() error: out of inotify entries #0 0x0000000000246b8f > > > llvm::sys::PrintStackTrace(llvm::raw_ostream&) > > > /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13 > > > #1 0x00000000002450d2 llvm::sys::RunSignalHandlers() > > > /mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18 > > > #2 0x0000000000247268 SignalHandler(int) > > > /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1 > > > #3 0x00007f17924eb890 __restore_rt > > > (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890) > > > #4 0x00007f1790f94e97 gsignal /build/glibc-OTsEL5/glibc- > > > ... > > > ``` > > > > > > Wrapping the llvm::Expected in a cantFail hides this message, and instead > > > you get the much less useful: > > > > > > ``` > > > Note: Google Test filter = DirectoryWatcherTest.AddFiles > > > [==========] Running 1 test from 1 test case. > > > > > > > > > > > > [----------] Global test environment set-up. > > > [----------] 1 test from DirectoryWatcherTest > > > [ RUN ] DirectoryWatcherTest.AddFiles > > > Failure value returned from cantFail wrapped call > > > UNREACHABLE executed at > > > /mnt/nvme0/llvm-project/llvm/include/llvm/Support/Error.h:707! > > > #0 0x0000000000246e0f llvm::sys::PrintStackTrace(llvm::raw_ostream&) > > > /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13 > > > #1 0x0000000000245352 llvm::sys::RunSignalHandlers() > > > /mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18 > > > #2 0x00000000002474e8 SignalHandler(int) > > > /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1 > > > #3 0x00007f20c64a8890 __restore_rt > > > (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890) > > > #4 0x00007f20c4f51e97 gsignal > > > /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0 > > > #5 0x00007f20c4f53801 abort > > > /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0 > > > #6 0x0000000000232b31 > > > (build/tools/clang/unittests/DirectoryWatcher/./DirectoryWatcherTests+0x232b31) > > > ``` > > > > > > I specifically _want_ llvm::Expected to propagate the error up. > > > I can drop the comments, because it should be fairly obvious here that DW > > > is an llvm::Expected since I got rid of the autos. > > > > > > To be clear, wrapping llvm::Expected in cantFail goes against the entire > > > point of why I added this more robust error handling. > > > If someone's machine is failing on this test because they've exceeded > > > their inotify limit, I would really like the message produced from > > > strerror(errno) to bubble up to the top so that it is obvious to them > > > that they need to either kill some tasks on their machine or up their > > > inotify limit. > > The automated checking in llvm::Expected is not guaranteed to happen (it is > > behind LLVM_ENABLE_ABI_BREAKING_CHECKS). If you are not happy with the > > error from `cantFail` I think everyone would appreciate an improvement > > there. > > > > > I specifically _want_ llvm::Expected to propagate the error up. > > > > I don't understand. It is not propagating anything up, it is just crashing > > in the destructor. > Also, I have to point out here: cantFail is exactly the wrong tool here > because this code can in fact fail if your system has exceeded its inotify > limits. For the purposes of the test, the call can't fail. If the call fails, the test fails. How about `ASSERT_TRUE(DW)`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65829/new/ https://reviews.llvm.org/D65829 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
