plotfi marked 2 inline comments as done. plotfi 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) ---------------- 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. 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
