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: > > 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. 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
