[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi added a comment.

@jkorous Can you lemme know if you're happy with 
https://reviews.llvm.org/D65829.
@gribozavr lemme also know any other feedback in D65829 
, as this diff here is already closed.


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Puyan Lotfi via Phabricator via cfe-commits 
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 must be checked before access or destruction.
> Unchecked Expected contained error:
> inotify_init1() error: out of inotify entries #0 0x00246b8f 
> llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
> /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13
>  #1 0x002450d2 llvm::sys::RunSignalHandlers() 
> /mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18
>  #2 0x00247268 SignalHandler(int) 
> /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1
>  #3 0x7f17924eb890 __restore_rt 
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
>  #4 0x7f1790f94e97 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 0x00246e0f llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
> /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13
>  #1 0x00245352 llvm::sys::RunSignalHandlers() 
> /mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18
>  #2 0x002474e8 SignalHandler(int) 
> /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1
>  #3 0x7f20c64a8890 __restore_rt 
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
>  #4 0x7f20c4f51e97 gsignal 
> /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
>  #5 0x7f20c4f53801 abort 
> /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0
>  #6 0x00232b31 
> (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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Puyan Lotfi via Phabricator via cfe-commits 
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;
 

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 must be checked before access or destruction.
Unchecked Expected contained error:
inotify_init1() error: out of inotify entries #0 0x00246b8f 
llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
/mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13
 #1 0x002450d2 llvm::sys::RunSignalHandlers() 
/mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18
 #2 0x00247268 SignalHandler(int) 
/mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1
 #3 0x7f17924eb890 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
 #4 0x7f1790f94e97 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 0x00246e0f llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
/mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13
 #1 0x00245352 llvm::sys::RunSignalHandlers() 
/mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18
 #2 0x002474e8 SignalHandler(int) 
/mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1
 #3 0x7f20c64a8890 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
 #4 0x7f20c4f51e97 gsignal 
/build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #5 0x7f20c4f53801 abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0
 #6 0x00232b31 
(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. 



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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Dmitri Gribenko via Phabricator via cfe-commits 
gribozavr 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:
> > > > 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.


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi marked 2 inline comments as done.
plotfi added inline comments.



Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start

plotfi wrote:
> gribozavr wrote:
> > jkorous wrote:
> > > plotfi wrote:
> > > > gribozavr wrote:
> > > > > I don't see where this logic is implemented.
> > > > > 
> > > > > Also, LLVM is often built with assertions disabled, so "asserts" 
> > > > > can't be a part of the contract.
> > > > Please be more specific. What logic are you talking about? 
> > > We shouldn't assert. Just return an error and let client code handle it 
> > > in any way it wants.
> > I'm saying that it is not appropriate to document the API as `assert()`ing 
> > about something, because it is not a part of the contract.
> > 
> > You can say that the API *requires* the Path to be non-empty. Or you can 
> > call llvm_fatal_error explicitly if it is empty, and then you can document 
> > it. Or you can return an error like jkorous said. However, assertions are 
> > not a part of the API contract.
> > 
> > 
> That makes sense. Lemme think about which is best, and I'll amend this diff.
@jkorous I have another patch ready that will replace the asserts with llvm 
fatal_error. If this is good by you I will land that.


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi marked an inline comment as done.
plotfi added inline comments.



Comment at: cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:329
+  assert(!Path.empty() && "Path.empty()");
 
   const int InotifyFD = inotify_init1(IN_CLOEXEC);


@gribozavr As an example, let say we stuck the following right here:


```
return llvm::make_error("llvm::Expected will blow up", 
llvm::inconvertibleErrorCode());
```

When running:



```
DirectoryWatcherTests --gtest_filter=DirectoryWatcherTest.AddFiles
```

You will get:

```
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 must be checked before access or destruction.
Unchecked Expected contained error:
llvm::Expected will blow up #0 0x00246b8f 
llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
/mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13
 #1 0x002450d2 llvm::sys::RunSignalHandlers() 
/mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18
 #2 0x00247268 SignalHandler(int) 
/mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1
 #3 0x7fa2a284d890 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
 #4 0x7fa2a12f6e97 gsignal 
/build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #5 0x7fa2a12f8801 abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0
 #6 0x002229db llvm::raw_ostream::operator<<(llvm::StringRef) 
/mnt/nvme0/llvm-project/llvm/include/llvm/Support/raw_ostream.h:176:7
 #7 0x002229db llvm::raw_ostream::operator<<(char const*) 
/mnt/nvme0/llvm-project/llvm/include/llvm/Support/raw_ostream.h:186:0
 #8 0x002229db llvm::Expected > >::fatalUncheckedExpected() 
const /mnt/nvme0/llvm-project/llvm/include/llvm/Support/Error.h:661:0
 #9 0x0021e148 
(build/tools/clang/unittests/DirectoryWatcher/./DirectoryWatcherTests+0x21e148)
#10 0x0024cbaa testing::internal::UnitTestImpl::os_stack_trace_getter() 
/mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4919:7
#11 0x0024cbaa testing::Test::Run() 
/mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2481:0
#12 0x0024d840 testing::internal::UnitTestImpl::os_stack_trace_getter() 
/mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4919:7
#13 0x0024d840 testing::TestInfo::Run() 
/mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2660:0
#14 0x0024dee7 testing::TestCase::Run() 
/mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2773:44
#15 0x002557b7 testing::internal::UnitTestImpl::RunAllTests() 
/mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4648:24
#16 0x00255411 testing::UnitTest::Run() 
/mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4257:10
#17 0x0024859b main 
/mnt/nvme0/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:3
#18 0x7fa2a12d9b97 __libc_start_main 
/build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:344:0
#19 0x0021b02a _start 
(build/tools/clang/unittests/DirectoryWatcher/./DirectoryWatcherTests+0x21b02a)
Aborted (core dumped)
```

This is preferable to deadlocking. 


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi marked an inline comment as done.
plotfi added inline comments.



Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

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. 


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi marked an inline comment as done.
plotfi added inline comments.



Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start

gribozavr wrote:
> jkorous wrote:
> > plotfi wrote:
> > > gribozavr wrote:
> > > > I don't see where this logic is implemented.
> > > > 
> > > > Also, LLVM is often built with assertions disabled, so "asserts" can't 
> > > > be a part of the contract.
> > > Please be more specific. What logic are you talking about? 
> > We shouldn't assert. Just return an error and let client code handle it in 
> > any way it wants.
> I'm saying that it is not appropriate to document the API as `assert()`ing 
> about something, because it is not a part of the contract.
> 
> You can say that the API *requires* the Path to be non-empty. Or you can call 
> llvm_fatal_error explicitly if it is empty, and then you can document it. Or 
> you can return an error like jkorous said. However, assertions are not a part 
> of the API contract.
> 
> 
That makes sense. Lemme think about which is best, and I'll amend this diff.


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Dmitri Gribenko via Phabricator via cfe-commits 
gribozavr added inline comments.



Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start

jkorous wrote:
> plotfi wrote:
> > gribozavr wrote:
> > > I don't see where this logic is implemented.
> > > 
> > > Also, LLVM is often built with assertions disabled, so "asserts" can't be 
> > > a part of the contract.
> > Please be more specific. What logic are you talking about? 
> We shouldn't assert. Just return an error and let client code handle it in 
> any way it wants.
I'm saying that it is not appropriate to document the API as `assert()`ing 
about something, because it is not a part of the contract.

You can say that the API *requires* the Path to be non-empty. Or you can call 
llvm_fatal_error explicitly if it is empty, and then you can document it. Or 
you can return an error like jkorous said. However, assertions are not a part 
of the API contract.





Comment at: cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:153
 dispatch_queue_t Queue) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 

jkorous wrote:
> plotfi wrote:
> > gribozavr wrote:
> > > No need to repeat the condition in the `&& "..."` part. assert does that 
> > > by default. Use an extra string to explain the assertion to the reader.
> > This is std assert. Are you referring to a different assert? If @compnerd 
> > is ok with changing the assert into an llvm::Expected I can do that. 
> I think the suggestion is just
> 
> ```
> assert(!Path.empty());
> ```
Right. The standard assert already prints the expression -- in fact, this is 
how the `&& "xyz"` part gets printed when assertion fails -- assert prints the 
expression that failed.



Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

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.


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Jan Korous via Phabricator via cfe-commits 
jkorous added inline comments.



Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start

plotfi wrote:
> gribozavr wrote:
> > I don't see where this logic is implemented.
> > 
> > Also, LLVM is often built with assertions disabled, so "asserts" can't be a 
> > part of the contract.
> Please be more specific. What logic are you talking about? 
We shouldn't assert. Just return an error and let client code handle it in any 
way it wants.



Comment at: cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:153
 dispatch_queue_t Queue) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 

plotfi wrote:
> gribozavr wrote:
> > No need to repeat the condition in the `&& "..."` part. assert does that by 
> > default. Use an extra string to explain the assertion to the reader.
> This is std assert. Are you referring to a different assert? If @compnerd is 
> ok with changing the assert into an llvm::Expected I can do that. 
I think the suggestion is just

```
assert(!Path.empty());
```


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi marked 6 inline comments as done.
plotfi added inline comments.



Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start

gribozavr wrote:
> I don't see where this logic is implemented.
> 
> Also, LLVM is often built with assertions disabled, so "asserts" can't be a 
> part of the contract.
Please be more specific. What logic are you talking about? 



Comment at: cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:153
 dispatch_queue_t Queue) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 

gribozavr wrote:
> No need to repeat the condition in the `&& "..."` part. assert does that by 
> default. Use an extra string to explain the assertion to the reader.
This is std assert. Are you referring to a different assert? If @compnerd is ok 
with changing the assert into an llvm::Expected I can do that. 



Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

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. 


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi marked an inline comment as done.
plotfi added inline comments.



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

gribozavr wrote:
> plotfi wrote:
> > jkorous wrote:
> > > jkorous wrote:
> > > > IIUC this is silently dropping errors. We should print the error here.
> > > Ah, my bad - I just took a better look at `Expected<>` and you're right.
> > Nah, the way llvm::Expected works is that if the error isn't consumed then 
> > it will blow up in the destructor. So if it is an error, returning will 
> > cause the destructor to crash the program and print the error implicitly. 
> > Very nice error handling mechanism you ask me :-) 
> And crashing would be much better in a test. The test should test the 
> DirectoryWatcher, not just be graceful about error handling.
Test was in a deadlock, see commit message. This fixes that.


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Dmitri Gribenko via Phabricator via cfe-commits 
gribozavr added inline comments.



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

plotfi wrote:
> jkorous wrote:
> > jkorous wrote:
> > > IIUC this is silently dropping errors. We should print the error here.
> > Ah, my bad - I just took a better look at `Expected<>` and you're right.
> Nah, the way llvm::Expected works is that if the error isn't consumed then it 
> will blow up in the destructor. So if it is an error, returning will cause 
> the destructor to crash the program and print the error implicitly. Very nice 
> error handling mechanism you ask me :-) 
And crashing would be much better in a test. The test should test the 
DirectoryWatcher, not just be graceful about error handling.


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-06 Thread Dmitri Gribenko via Phabricator via cfe-commits 
gribozavr added inline comments.



Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start

I don't see where this logic is implemented.

Also, LLVM is often built with assertions disabled, so "asserts" can't be a 
part of the contract.



Comment at: cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:153
 dispatch_queue_t Queue) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 

No need to repeat the condition in the `&& "..."` part. assert does that by 
default. Use an extra string to explain the assertion to the reader.



Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

Why? This is a test -- ignoring errors would only hide them from developers.


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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-05 Thread Puyan Lotfi via Phabricator via cfe-commits 
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL367979: [clang][DirectoryWatcher] Adding llvm::Expected 
error handling to create. (authored by zer0, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D65704?vs=213520=213521#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65704/new/

https://reviews.llvm.org/D65704

Files:
  cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
  cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
  cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
===
--- cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
+++ cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
@@ -11,6 +11,7 @@
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include 
 #include 
 #include 
@@ -98,10 +99,11 @@
 : Kind(Kind), Filename(Filename) {}
   };
 
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start
+  /// watching. In such case it's unclear whether just retrying has any chance
+  /// to succeeed.
+  static llvm::Expected>
   create(llvm::StringRef Path,
  std::function Events,
 bool IsInitial)>
Index: cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
===
--- cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
+++ cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
@@ -11,9 +11,11 @@
 using namespace llvm;
 using namespace clang;
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
-  return nullptr;
+  return llvm::make_error(
+  "DirectoryWatcher is not implemented for this platform!",
+  llvm::inconvertibleErrorCode());
 }
\ No newline at end of file
Index: cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
===
--- cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
+++ cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
@@ -11,16 +11,13 @@
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include 
 
 using namespace llvm;
 using namespace clang;
 
-static FSEventStreamRef createFSEventStream(
-StringRef Path,
-std::function, bool)>,
-dispatch_queue_t);
 static void stopFSEventStream(FSEventStreamRef);
 
 namespace {
@@ -153,8 +150,7 @@
 StringRef Path,
 std::function, bool)> Receiver,
 dispatch_queue_t Queue) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 
   CFMutableArrayRef PathsToWatch = [&]() {
 CFMutableArrayRef PathsToWatch =
@@ -205,20 +201,16 @@
   FSEventStreamRelease(EventStream);
 }
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
   dispatch_queue_t Queue =
   dispatch_queue_create("DirectoryWatcher", DISPATCH_QUEUE_SERIAL);
 
-  if (Path.empty())
-return nullptr;
-
+  assert(!Path.empty() && "Path.empty()");
   auto EventStream = createFSEventStream(Path, Receiver, Queue);
-  if (!EventStream) {
-return nullptr;
-  }
+  assert(EventStream && "EventStream expected to be non-null.")
 
   std::unique_ptr Result =
   llvm::make_unique(EventStream, Receiver, Path);
Index: cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
===
--- cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
+++ cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/Path.h"
 #include 
@@ -320,16 +321,17 @@
 
 } // namespace
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected>

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-05 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi updated this revision to Diff 213520.
plotfi added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

add another assert


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65704/new/

https://reviews.llvm.org/D65704

Files:
  include/clang/DirectoryWatcher/DirectoryWatcher.h
  lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
  lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
+++ unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -284,6 +284,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   checkEventualResultWithTimeout(TestConsumer);
 }
@@ -315,6 +316,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/false);
+  if (!DW) return;
 
   checkEventualResultWithTimeout(TestConsumer);
 }
@@ -335,6 +337,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   fixture.addFile("a");
   fixture.addFile("b");
@@ -360,6 +363,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   // modify the file
   {
@@ -390,6 +394,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   fixture.deleteFile("a");
 
@@ -411,6 +416,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   remove_directories(fixture.TestWatchedDir);
 
@@ -431,6 +437,7 @@
   TestConsumer.consume(Events, IsInitial);
 },
 /*waitForInitialSync=*/true);
+if (!DW) return;
   } // DW is destructed here.
 
   checkEventualResultWithTimeout(TestConsumer);
Index: lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
===
--- lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
+++ lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
@@ -11,6 +11,7 @@
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include 
 
@@ -17,10 +18,6 @@
 using namespace llvm;
 using namespace clang;
 
-static FSEventStreamRef createFSEventStream(
-StringRef Path,
-std::function, bool)>,
-dispatch_queue_t);
 static void stopFSEventStream(FSEventStreamRef);
 
 namespace {
@@ -153,8 +150,7 @@
 StringRef Path,
 std::function, bool)> Receiver,
 dispatch_queue_t Queue) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 
   CFMutableArrayRef PathsToWatch = [&]() {
 CFMutableArrayRef PathsToWatch =
@@ -205,7 +201,7 @@
   FSEventStreamRelease(EventStream);
 }
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
@@ -212,13 +208,9 @@
   dispatch_queue_t Queue =
   dispatch_queue_create("DirectoryWatcher", DISPATCH_QUEUE_SERIAL);
 
-  if (Path.empty())
-return nullptr;
-
+  assert(!Path.empty() && "Path.empty()");
   auto EventStream = createFSEventStream(Path, Receiver, Queue);
-  if (!EventStream) {
-return nullptr;
-  }
+  assert(EventStream && "EventStream expected to be non-null.")
 
   std::unique_ptr Result =
   llvm::make_unique(EventStream, Receiver, Path);
Index: lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
===
--- lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
+++ lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/Path.h"
 #include 
@@ -320,16 +321,17 @@
 
 } // namespace
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 
   const int InotifyFD = inotify_init1(IN_CLOEXEC);
   if (InotifyFD == -1)
-return nullptr;
+return llvm::make_error(
+std::string("inotify_init1() error: ") + strerror(errno),
+llvm::inconvertibleErrorCode());
 
   const int InotifyWD = inotify_add_watch(
   InotifyFD, Path.str().c_str(),
@@ -340,12 +342,16 @@
 #endif
   );
   if (InotifyWD == -1)
-return

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-05 Thread Saleem Abdulrasool via Phabricator via cfe-commits 
compnerd added inline comments.



Comment at: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:218
+return llvm::make_error(
+std::string("Path.empty() error: ") + strerror(errno),
+llvm::inconvertibleErrorCode());

compnerd wrote:
> Similar.
Should this just assert as well?


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-05 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi updated this revision to Diff 213515.
plotfi added a comment.

Improve comments, change Path.empty() errors for asserts.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65704/new/

https://reviews.llvm.org/D65704

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -280,6 +280,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   checkEventualResultWithTimeout(TestConsumer);
 }
@@ -311,6 +312,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/false);
+  if (!DW) return;
 
   checkEventualResultWithTimeout(TestConsumer);
 }
@@ -331,6 +333,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   fixture.addFile("a");
   fixture.addFile("b");
@@ -356,6 +359,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   // modify the file
   {
@@ -386,6 +390,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   fixture.deleteFile("a");
 
@@ -407,6 +412,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   remove_directories(fixture.TestWatchedDir);
 
@@ -427,7 +433,8 @@
   TestConsumer.consume(Events, IsInitial);
 },
 /*waitForInitialSync=*/true);
+if (!DW) return;
   } // DW is destructed here.
 
   checkEventualResultWithTimeout(TestConsumer);
 }
\ No newline at end of file
Index: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
===
--- clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
+++ clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
@@ -11,16 +11,13 @@
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include 
 
 using namespace llvm;
 using namespace clang;
 
-static FSEventStreamRef createFSEventStream(
-StringRef Path,
-std::function, bool)>,
-dispatch_queue_t);
 static void stopFSEventStream(FSEventStreamRef);
 
 namespace {
@@ -153,8 +150,7 @@
 StringRef Path,
 std::function, bool)> Receiver,
 dispatch_queue_t Queue) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 
   CFMutableArrayRef PathsToWatch = [&]() {
 CFMutableArrayRef PathsToWatch =
@@ -205,7 +201,7 @@
   FSEventStreamRelease(EventStream);
 }
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
@@ -213,12 +209,11 @@
   dispatch_queue_create("DirectoryWatcher", DISPATCH_QUEUE_SERIAL);
 
   if (Path.empty())
-return nullptr;
+return llvm::make_error(
+std::string("Path.empty() error: "), llvm::inconvertibleErrorCode());
 
   auto EventStream = createFSEventStream(Path, Receiver, Queue);
-  if (!EventStream) {
-return nullptr;
-  }
+  assert(EventStream && "EventStream expected to be non-null.")
 
   std::unique_ptr Result =
   llvm::make_unique(EventStream, Receiver, Path);
Index: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
===
--- clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
+++ clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/Path.h"
 #include 
@@ -320,16 +321,17 @@
 
 } // namespace
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 
   const int InotifyFD = inotify_init1(IN_CLOEXEC);
   if (InotifyFD == -1)
-return nullptr;
+return llvm::make_error(
+std::string("inotify_init1() error: ") + strerror(errno),
+llvm::inconvertibleErrorCode());
 
   const int InotifyWD = inotify_add_watch(

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-05 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi marked 3 inline comments as done.
plotfi added inline comments.



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
   /// Returns nullptr if \param Path doesn't exist or isn't a directory.
   /// Returns nullptr if OS kernel API told us we can't start watching. In such

jkorous wrote:
> Could you please update the comments?
Ahh, Yes. 



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

jkorous wrote:
> jkorous wrote:
> > IIUC this is silently dropping errors. We should print the error here.
> Ah, my bad - I just took a better look at `Expected<>` and you're right.
Nah, the way llvm::Expected works is that if the error isn't consumed then it 
will blow up in the destructor. So if it is an error, returning will cause the 
destructor to crash the program and print the error implicitly. Very nice error 
handling mechanism you ask me :-) 


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-05 Thread Jan Korous via Phabricator via cfe-commits 
jkorous added a comment.

Please just update the comment, otherwise LGTM.




Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

jkorous wrote:
> IIUC this is silently dropping errors. We should print the error here.
Ah, my bad - I just took a better look at `Expected<>` and you're right.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-05 Thread Jan Korous via Phabricator via cfe-commits 
jkorous added a comment.

Thanks for the patch!




Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
   /// Returns nullptr if \param Path doesn't exist or isn't a directory.
   /// Returns nullptr if OS kernel API told us we can't start watching. In such

Could you please update the comments?



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

IIUC this is silently dropping errors. We should print the error here.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-03 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi updated this revision to Diff 213221.
plotfi added a comment.

Fix a linux typo


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65704/new/

https://reviews.llvm.org/D65704

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -280,6 +280,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   checkEventualResultWithTimeout(TestConsumer);
 }
@@ -311,6 +312,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/false);
+  if (!DW) return;
 
   checkEventualResultWithTimeout(TestConsumer);
 }
@@ -331,6 +333,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   fixture.addFile("a");
   fixture.addFile("b");
@@ -356,6 +359,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   // modify the file
   {
@@ -386,6 +390,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   fixture.deleteFile("a");
 
@@ -407,6 +412,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   remove_directories(fixture.TestWatchedDir);
 
@@ -427,7 +433,8 @@
   TestConsumer.consume(Events, IsInitial);
 },
 /*waitForInitialSync=*/true);
+if (!DW) return;
   } // DW is destructed here.
 
   checkEventualResultWithTimeout(TestConsumer);
 }
\ No newline at end of file
Index: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
===
--- clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
+++ clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
@@ -11,16 +11,13 @@
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include 
 
 using namespace llvm;
 using namespace clang;
 
-static FSEventStreamRef createFSEventStream(
-StringRef Path,
-std::function, bool)>,
-dispatch_queue_t);
 static void stopFSEventStream(FSEventStreamRef);
 
 namespace {
@@ -131,12 +128,13 @@
   }
 }
 
-FSEventStreamRef createFSEventStream(
+llvm::Expected createFSEventStream(
 StringRef Path,
 std::function, bool)> Receiver,
 dispatch_queue_t Queue) {
   if (Path.empty())
-return nullptr;
+return llvm::make_error(
+std::string("Path.empty() error: "), llvm::inconvertibleErrorCode());
 
   CFMutableArrayRef PathsToWatch = [&]() {
 CFMutableArrayRef PathsToWatch =
@@ -187,7 +185,7 @@
   FSEventStreamRelease(EventStream);
 }
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
@@ -195,12 +193,14 @@
   dispatch_queue_create("DirectoryWatcher", DISPATCH_QUEUE_SERIAL);
 
   if (Path.empty())
-return nullptr;
+return llvm::make_error(
+std::string("Path.empty() error: "), llvm::inconvertibleErrorCode());
+
+  auto EventStreamOrErr = createFSEventStream(Path, Receiver, Queue);
+  if (!EventStreamOrErr)
+return EventStreamOrErr.takeError();
+  auto EventStream = EventStreamOrErr.get();
 
-  auto EventStream = createFSEventStream(Path, Receiver, Queue);
-  if (!EventStream) {
-return nullptr;
-  }
 
   std::unique_ptr Result =
   llvm::make_unique(EventStream, Receiver, Path);
Index: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
===
--- clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
+++ clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/Path.h"
 #include 
@@ -321,16 +322,19 @@
 
 } // namespace
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
   if (Path.empty())
-return nullptr;
+return llvm::make_error(
+std::string("Path.empty() error: "),

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-03 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi added a comment.

In D65704#1613688 , @compnerd wrote:

> Rather than silently ignoring tests when the DirectoryWatcher isn't created, 
> can you please print an error message and exit with an error code to indicate 
> the test failed?


llvm::Expected requires consumption.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-03 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi updated this revision to Diff 213218.
plotfi added a comment.

More cleanup


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65704/new/

https://reviews.llvm.org/D65704

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -280,6 +280,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   checkEventualResultWithTimeout(TestConsumer);
 }
@@ -311,6 +312,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/false);
+  if (!DW) return;
 
   checkEventualResultWithTimeout(TestConsumer);
 }
@@ -331,6 +333,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   fixture.addFile("a");
   fixture.addFile("b");
@@ -356,6 +359,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   // modify the file
   {
@@ -386,6 +390,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   fixture.deleteFile("a");
 
@@ -407,6 +412,7 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 
   remove_directories(fixture.TestWatchedDir);
 
@@ -427,7 +433,8 @@
   TestConsumer.consume(Events, IsInitial);
 },
 /*waitForInitialSync=*/true);
+if (!DW) return;
   } // DW is destructed here.
 
   checkEventualResultWithTimeout(TestConsumer);
 }
\ No newline at end of file
Index: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
===
--- clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
+++ clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
@@ -11,16 +11,13 @@
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include 
 
 using namespace llvm;
 using namespace clang;
 
-static FSEventStreamRef createFSEventStream(
-StringRef Path,
-std::function, bool)>,
-dispatch_queue_t);
 static void stopFSEventStream(FSEventStreamRef);
 
 namespace {
@@ -149,12 +146,13 @@
   }
 }
 
-FSEventStreamRef createFSEventStream(
+llvm::Expected createFSEventStream(
 StringRef Path,
 std::function, bool)> Receiver,
 dispatch_queue_t Queue) {
   if (Path.empty())
-return nullptr;
+return llvm::make_error(
+std::string("Path.empty() error: "), llvm::inconvertibleErrorCode());
 
   CFMutableArrayRef PathsToWatch = [&]() {
 CFMutableArrayRef PathsToWatch =
@@ -205,7 +203,7 @@
   FSEventStreamRelease(EventStream);
 }
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
@@ -213,12 +211,14 @@
   dispatch_queue_create("DirectoryWatcher", DISPATCH_QUEUE_SERIAL);
 
   if (Path.empty())
-return nullptr;
+return llvm::make_error(
+std::string("Path.empty() error: "), llvm::inconvertibleErrorCode());
+
+  auto EventStreamOrErr = createFSEventStream(Path, Receiver, Queue);
+  if (!EventStreamOrErr)
+return EventStreamOrErr.takeError();
+  auto EventStream = EventStreamOrErr.get();
 
-  auto EventStream = createFSEventStream(Path, Receiver, Queue);
-  if (!EventStream) {
-return nullptr;
-  }
 
   std::unique_ptr Result =
   llvm::make_unique(EventStream, Receiver, Path);
Index: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
===
--- clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
+++ clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/Path.h"
 #include 
@@ -321,16 +322,19 @@
 
 } // namespace
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
   if (Path.empty())
-return nullptr;
+return llvm::make_error(
+std::string("Path.empty() error: "), llvm::inconvertibleErrorCode());

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-03 Thread Saleem Abdulrasool via Phabricator via cfe-commits 
compnerd added a comment.

BTW, I think that we should add a test case to ensure that we see the error in 
the case that the inotify fds are exhausted.  We should be able to create a 
process and set the limit for that process to 0/1 and use that to trigger the 
failure.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-03 Thread Saleem Abdulrasool via Phabricator via cfe-commits 
compnerd added a comment.

Rather than silently ignoring tests when the DirectoryWatcher isn't created, 
can you please print an error message and exit with an error code to indicate 
the test failed?




Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:331
+return llvm::make_error(
+std::string("Path.empty() error: ") + strerror(errno),
+llvm::inconvertibleErrorCode());

I think that this is pointless.  Just make it a constant string: "no path 
specified".  `Path` is a user specified parameter, `errno` doesn't tell us 
anything.



Comment at: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:218
+return llvm::make_error(
+std::string("Path.empty() error: ") + strerror(errno),
+llvm::inconvertibleErrorCode());

Similar.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-03 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi updated this revision to Diff 213209.
plotfi added a comment.

Cleanup some stuff


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65704/new/

https://reviews.llvm.org/D65704

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -280,8 +280,8 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
-
-  checkEventualResultWithTimeout(TestConsumer);
+  if (DW)
+checkEventualResultWithTimeout(TestConsumer);
 }
 
 TEST(DirectoryWatcherTest, InitialScanAsync) {
@@ -311,8 +311,8 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/false);
-
-  checkEventualResultWithTimeout(TestConsumer);
+  if (DW)
+checkEventualResultWithTimeout(TestConsumer);
 }
 
 TEST(DirectoryWatcherTest, AddFiles) {
@@ -331,12 +331,12 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
-
-  fixture.addFile("a");
-  fixture.addFile("b");
-  fixture.addFile("c");
-
-  checkEventualResultWithTimeout(TestConsumer);
+  if (DW) {
+fixture.addFile("a");
+fixture.addFile("b");
+fixture.addFile("c");
+checkEventualResultWithTimeout(TestConsumer);
+  }
 }
 
 TEST(DirectoryWatcherTest, ModifyFile) {
@@ -356,17 +356,18 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (DW) {
+// modify the file
+{
+  std::error_code error;
+  llvm::raw_fd_ostream bStream(fixture.getPathInWatched("a"), error,
+   CD_OpenExisting);
+  assert(!error);
+  bStream << "foo";
+}
 
-  // modify the file
-  {
-std::error_code error;
-llvm::raw_fd_ostream bStream(fixture.getPathInWatched("a"), error,
- CD_OpenExisting);
-assert(!error);
-bStream << "foo";
+checkEventualResultWithTimeout(TestConsumer);
   }
-
-  checkEventualResultWithTimeout(TestConsumer);
 }
 
 TEST(DirectoryWatcherTest, DeleteFile) {
@@ -386,10 +387,10 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
-
-  fixture.deleteFile("a");
-
-  checkEventualResultWithTimeout(TestConsumer);
+  if (DW) {
+fixture.deleteFile("a");
+checkEventualResultWithTimeout(TestConsumer);
+  }
 }
 
 TEST(DirectoryWatcherTest, DeleteWatchedDir) {
@@ -407,10 +408,10 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
-
-  remove_directories(fixture.TestWatchedDir);
-
-  checkEventualResultWithTimeout(TestConsumer);
+  if (DW) {
+remove_directories(fixture.TestWatchedDir);
+checkEventualResultWithTimeout(TestConsumer);
+  }
 }
 
 TEST(DirectoryWatcherTest, InvalidatedWatcher) {
@@ -427,7 +428,9 @@
   TestConsumer.consume(Events, IsInitial);
 },
 /*waitForInitialSync=*/true);
+if (DW)
+  return;
   } // DW is destructed here.
 
   checkEventualResultWithTimeout(TestConsumer);
 }
\ No newline at end of file
Index: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
===
--- clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
+++ clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
@@ -11,6 +11,7 @@
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include 
 
@@ -205,7 +206,7 @@
   FSEventStreamRelease(EventStream);
 }
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
@@ -213,12 +214,15 @@
   dispatch_queue_create("DirectoryWatcher", DISPATCH_QUEUE_SERIAL);
 
   if (Path.empty())
-return nullptr;
+return llvm::make_error(
+std::string("Path.empty() error: ") + strerror(errno),
+llvm::inconvertibleErrorCode());
 
   auto EventStream = createFSEventStream(Path, Receiver, Queue);
-  if (!EventStream) {
-return nullptr;
-  }
+  if (!EventStream)
+return llvm::make_error(
+std::string("createFSEventStream() error: ") + strerror(errno),
+llvm::inconvertibleErrorCode());
 
   std::unique_ptr Result =
   llvm::make_unique(EventStream, Receiver, Path);
Index: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-03 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi marked an inline comment as done.
plotfi added inline comments.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:32
 #include 
+#include 
 

This include should be removed. 


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

2019-08-03 Thread Puyan Lotfi via Phabricator via cfe-commits 
plotfi created this revision.
plotfi added reviewers: jkorous, compnerd.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.

There are cases where the DirectoryWatcherTests just hang in a deadlock when 
there are inotify limits hit, with no error reporting. This patch is a first 
attempt to resolve these issues by bubbling up errors to the user.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65704

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -280,6 +280,10 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) {
+llvm::errs() << llvm::toString(DW.takeError()) << "\n";
+llvm_unreachable("DirectoryWatcher::create() FAILED!");
+  }
 
   checkEventualResultWithTimeout(TestConsumer);
 }
@@ -311,6 +315,10 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/false);
+  if (!DW) {
+llvm::errs() << llvm::toString(DW.takeError()) << "\n";
+llvm_unreachable("DirectoryWatcher::create() FAILED!");
+  }
 
   checkEventualResultWithTimeout(TestConsumer);
 }
@@ -331,6 +339,10 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) {
+llvm::errs() << llvm::toString(DW.takeError()) << "\n";
+llvm_unreachable("DirectoryWatcher::create() FAILED!");
+  }
 
   fixture.addFile("a");
   fixture.addFile("b");
@@ -356,6 +368,10 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) {
+llvm::errs() << llvm::toString(DW.takeError()) << "\n";
+llvm_unreachable("DirectoryWatcher::create() FAILED!");
+  }
 
   // modify the file
   {
@@ -386,6 +402,10 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) {
+llvm::errs() << llvm::toString(DW.takeError()) << "\n";
+llvm_unreachable("DirectoryWatcher::create() FAILED!");
+  }
 
   fixture.deleteFile("a");
 
@@ -407,6 +427,10 @@
 TestConsumer.consume(Events, IsInitial);
   },
   /*waitForInitialSync=*/true);
+  if (!DW) {
+llvm::errs() << llvm::toString(DW.takeError()) << "\n";
+llvm_unreachable("DirectoryWatcher::create() FAILED!");
+  }
 
   remove_directories(fixture.TestWatchedDir);
 
@@ -427,7 +451,11 @@
   TestConsumer.consume(Events, IsInitial);
 },
 /*waitForInitialSync=*/true);
+if (!DW) {
+  llvm::errs() << llvm::toString(DW.takeError()) << "\n";
+  llvm_unreachable("DirectoryWatcher::create() FAILED!");
+}
   } // DW is destructed here.
 
   checkEventualResultWithTimeout(TestConsumer);
 }
\ No newline at end of file
Index: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
===
--- clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
+++ clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
@@ -11,6 +11,7 @@
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include 
 
@@ -205,7 +206,7 @@
   FSEventStreamRelease(EventStream);
 }
 
-std::unique_ptr clang::DirectoryWatcher::create(
+llvm::Expected> clang::DirectoryWatcher::create(
 StringRef Path,
 std::function, bool)> Receiver,
 bool WaitForInitialSync) {
@@ -213,12 +214,15 @@
   dispatch_queue_create("DirectoryWatcher", DISPATCH_QUEUE_SERIAL);
 
   if (Path.empty())
-return nullptr;
+return llvm::make_error(
+std::string("Path.empty() error: ") + strerror(errno),
+llvm::inconvertibleErrorCode());
 
   auto EventStream = createFSEventStream(Path, Receiver, Queue);
-  if (!EventStream) {
-return nullptr;
-  }
+  if (!EventStream)
+return llvm::make_error(
+std::string("createFSEventStream() error: ") + strerror(errno),
+llvm::inconvertibleErrorCode());
 
   std::unique_ptr Result =
   llvm::make_unique(EventStream, Receiver, Path);
Index: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
===
--- clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
+++ clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
 #include