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:
> > > gribozavr wrote:
> > > > plotfi wrote:
> > > > > gribozavr wrote:
> > > > > > Why? This is a test -- ignoring errors would only hide them from
> > > > > > developers.
> > > > > Please see how llvm::Expected works. This does not ignore or hide
> > > > > anything.
> > > > I think I know how llvm::Expected works.
> > > >
> > > > The problem that this check and return hides is that if
> > > > DirectoryWatcher::create returns an error, the rest of the test is
> > > > silently slipped. The test should be using something like `cantFail`
> > > > instead.
> > > If DirectoryWatcher returns an error and you hit the early return,
> > > without taking the error the Expected destructor will print a callstack
> > > and halt the program. I can check this again, but I am quite sure that
> > > without invoking something like takeError you will hit the crash in the
> > > case of an early return.
> > That's good -- but I think a `cantFail` call would be more readable.
> Ah, you should have said so then. Except this is arguably worse. I want this
> test to tell me that 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 can add comments assuring that llvm::Expected will dump an error prior to
> halting, but I specifically _want_ llvm::Expected to propagate up what perror
> would print to the console otherwise.
>
To be clear, I want the error to tell the programmer what is going on so that
they can either figure out what is eating up their machine resources and either
modify a sysctl entry or kill some process.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65704/new/
https://reviews.llvm.org/D65704
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
