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: > > > > 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)`? > I meant, it forces the code to either have to takeError and propagate the > error up or it will crash in the destructor. As far as I can tell that's how > it's supposed to work. I want the error produced inside > DirectoryWatcher::create to show up in the user's console. > I want the error produced inside DirectoryWatcher::create to show up in the > user's console. But the destructor of `llvm::Expected` is not guaranteed to do that. It will only detect (and print) the error under `LLVM_ENABLE_ABI_BREAKING_CHECKS`. This is exactly why I'm suggesting various alternative solutions. 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
