Re: [PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Puyan Lotfi via cfe-commits]

https://reviews.llvm.org/D65704
https://reviews.llvm.org/D65708

Thanks!

PL

On Thu, Aug 1, 2019 at 5:42 PM Jan Korous  wrote:

> Oh! Interesting! Thanks for investigating this.
>
> I'm happy to review the patch.
>
> On Aug 1, 2019, at 5:17 PM, Puyan Lotfi 
> wrote:
>
> Hi Jan, Thanks for the follow up!
>
> Me and Compnerd narrowed down the issue to the inotify file count limit
> being exceeded as the cause. DirectoryWatcherLinux::create() in
> DirectoryWatcher-linux.cpp also doesn't properly perror, I'll post a patch
> for this shortly to print perror info if the file count is exceeded.
>
>
> The cause of the inotify limit being exceeded was... drumroll... _Dropbox_
>
> PL
>
>
>
> On Thu, Aug 1, 2019 at 11:24 AM Jan Korous via Phabricator via
> llvm-commits  wrote:
>
>> jkorous added a comment.
>>
>> Hi Puyan,
>>
>> I failed to reproduce with llvm.org/master
>> (5faa533e47b0e54b04166b0257c5ebb48e6ffcaa <
>> https://reviews.llvm.org/rG5faa533e47b0e54b04166b0257c5ebb48e6ffcaa>) on
>> Ubuntu 18.04.1 LTS both in debug and release build.
>>
>> Since it sounds like you can reproduce "reliably" - can you please share
>> more info how to reproduce?
>>
>>
>> Repository:
>>   rL LLVM
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D58418/new/
>>
>> https://reviews.llvm.org/D58418
>>
>>
>>
>> ___
>> llvm-commits mailing list
>> llvm-comm...@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Puyan Lotfi via cfe-commits]

Hi Jan, Thanks for the follow up!

Me and Compnerd narrowed down the issue to the inotify file count limit
being exceeded as the cause. DirectoryWatcherLinux::create() in
DirectoryWatcher-linux.cpp also doesn't properly perror, I'll post a patch
for this shortly to print perror info if the file count is exceeded.

The cause of the inotify limit being exceeded was... drumroll... _Dropbox_

PL



On Thu, Aug 1, 2019 at 11:24 AM Jan Korous via Phabricator via llvm-commits
 wrote:

> jkorous added a comment.
>
> Hi Puyan,
>
> I failed to reproduce with llvm.org/master
> (5faa533e47b0e54b04166b0257c5ebb48e6ffcaa <
> https://reviews.llvm.org/rG5faa533e47b0e54b04166b0257c5ebb48e6ffcaa>) on
> Ubuntu 18.04.1 LTS both in debug and release build.
>
> Since it sounds like you can reproduce "reliably" - can you please share
> more info how to reproduce?
>
>
> Repository:
>   rL LLVM
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D58418/new/
>
> https://reviews.llvm.org/D58418
>
>
>
> ___
> llvm-commits mailing list
> llvm-comm...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via cfe-commits]

Oh! Interesting! Thanks for investigating this.

I'm happy to review the patch.

> On Aug 1, 2019, at 5:17 PM, Puyan Lotfi  wrote:
> 
> Hi Jan, Thanks for the follow up!
> 
> Me and Compnerd narrowed down the issue to the inotify file count limit being 
> exceeded as the cause. DirectoryWatcherLinux::create() in 
> DirectoryWatcher-linux.cpp also doesn't properly perror, I'll post a patch 
> for this shortly to print perror info if the file count is exceeded.
> 
> The cause of the inotify limit being exceeded was... drumroll... _Dropbox_ 
> 
> PL
> 
> 
> 
> On Thu, Aug 1, 2019 at 11:24 AM Jan Korous via Phabricator via llvm-commits 
> mailto:llvm-comm...@lists.llvm.org>> wrote:
> jkorous added a comment.
> 
> Hi Puyan,
> 
> I failed to reproduce with llvm.org/master  
> (5faa533e47b0e54b04166b0257c5ebb48e6ffcaa 
>  >) on 
> Ubuntu 18.04.1 LTS both in debug and release build.
> 
> Since it sounds like you can reproduce "reliably" - can you please share more 
> info how to reproduce?
> 
> 
> Repository:
>   rL LLVM
> 
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D58418/new/ 
> 
> https://reviews.llvm.org/D58418 
> 
> 
> 
> ___
> llvm-commits mailing list
> llvm-comm...@lists.llvm.org 
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Hi Puyan,

I failed to reproduce with llvm.org/master 
(5faa533e47b0e54b04166b0257c5ebb48e6ffcaa 
) on 
Ubuntu 18.04.1 LTS both in debug and release build.

Since it sounds like you can reproduce "reliably" - can you please share more 
info how to reproduce?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D58418#1609138 , @plotfi wrote:

> @jkorous DirectoryWatcherTests causes ninja check-clang to hang on my Ubuntu 
> 18.04 computer. check-clang will not finish and I am forced to killall -9 
> DirectoryWatcherTests. My system has 40 threads and this repros on ext4 and 
> btrfs.


Hi @plotfi,

Could you please add more details?

The tests seem fine on CentOS 6.9 with Linux 4.19.34-77 on ext4 and also Ubuntu 
build bots (Ubuntu 18.04.1 LTS) seem fine.
http://lab.llvm.org:8011/buildslaves/ps4-buildslave1a

Unfortunately I don't have any Ubuntu system at hand.

Thanks.

Jan


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

@jkorous DirectoryWatcherTests causes ninja check-clang to hang on my Ubuntu 
18.04 computer. check-clang will not finish and I am forced to killall -9 
DirectoryWatcherTests. My system has 40 threads and this repros on ext4 and 
btrfs.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added subscribers: beanz, smeenai.
smeenai added a comment.

In D58418#1577349 , @jkorous wrote:

> Thanks for the revert.
>
> There's actually one more problem - seems like ninja doesn't like the 
> generated build.ninja file on Linux.
>
>   ninja: error: build.ninja:52390: bad $-escape (literal $ must be written as 
> $$)
>
>
> http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/13617/steps/build-stage1-compiler/logs/stdio


We ran into this as well, and I was curious about what was going on, so I dug 
into it.

When you add a private dependency to static library target, as you're doing in 
this change with `${DIRECTORY_WATCHER_LINK_LIBS}`, CMake adds the dependency to 
the target's INTERFACE_LINK_LIBRARIES property using the `$` 
generator expression. The code for clang-shlib iterates through all the clang 
libraries and uses generator expressions to gather their 
INTERFACE_LINK_LIBRARIES, but if those generator expressions themselves 
evaluate to generator expressions (`$` in this case), the second 
level of generator expressions won't get evaluated and will just end up in the 
ninja file directly, hence the complaint about the dollar. The clang-shlib code 
in question is 
https://github.com/llvm/llvm-project/blob/3837f4273fcc40cc519035479aefe78e5cbd3055/clang/tools/clang-shlib/CMakeLists.txt#L10.

Here's a simple repro (where `empty.c` is literally an empty file). Running 
`cmake -G Ninja` on this and then running `ninja` should demonstrate the issue.

  cmake_minimum_required(VERSION 3.4.3)
  project(dollartest C)
  
  add_library(a STATIC empty.c)
  add_library(b_obj OBJECT empty.c)
  add_library(b STATIC empty.c)
  target_link_libraries(b PRIVATE a)
  
  add_library(c SHARED empty.c)
  target_link_libraries(c PRIVATE
b_object
$
$
)

@beanz, thoughts on how best to handle this?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Thanks for the revert.

There's actually one more problem - seems like ninja doesn't like the generated 
build.ninja file on Linux.

  ninja: error: build.ninja:52390: bad $-escape (literal $ must be written as 
$$)

http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/13617/steps/build-stage1-compiler/logs/stdio


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Reverted in rC365581 .


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:420
+std::error_code setTimeRes =
+llvm::sys::fs::setLastAccessAndModificationTime(FD, NewTimePt,
+NewTimePt);

This fails to compile on Windows because file_t is not int there:
C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\unittests\DirectoryWatcher\DirectoryWatcherTest.cpp(415):
 error C2440: 'initializing': cannot convert from 'void *' to 'int'
C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\unittests\DirectoryWatcher\DirectoryWatcherTest.cpp(415):
 note: There is no context in which this conversion is possible

I have been working on migrating some code over to native file handles to make 
this type of error less likely in the future, but it is not done yet.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365574: [clang] DirectoryWatcher (authored by jkorous, 
committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58418?vs=202833=208833#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58418

Files:
  cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
  cfe/trunk/lib/CMakeLists.txt
  cfe/trunk/lib/DirectoryWatcher/CMakeLists.txt
  cfe/trunk/lib/DirectoryWatcher/DirectoryScanner.cpp
  cfe/trunk/lib/DirectoryWatcher/DirectoryScanner.h
  cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  cfe/trunk/unittests/CMakeLists.txt
  cfe/trunk/unittests/DirectoryWatcher/CMakeLists.txt
  cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

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
@@ -0,0 +1,233 @@
+//===- DirectoryWatcher-mac.cpp - Mac-platform directory watching -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "DirectoryScanner.h"
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.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 {
+
+class DirectoryWatcherMac : public clang::DirectoryWatcher {
+public:
+  DirectoryWatcherMac(
+  FSEventStreamRef EventStream,
+  std::function, bool)>
+  Receiver,
+  llvm::StringRef WatchedDirPath)
+  : EventStream(EventStream), Receiver(Receiver),
+WatchedDirPath(WatchedDirPath) {}
+
+  ~DirectoryWatcherMac() override {
+stopFSEventStream(EventStream);
+EventStream = nullptr;
+// Now it's safe to use Receiver as the only other concurrent use would have
+// been in EventStream processing.
+Receiver(DirectoryWatcher::Event(
+ DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""),
+ false);
+  }
+
+private:
+  FSEventStreamRef EventStream;
+  std::function, bool)> Receiver;
+  const std::string WatchedDirPath;
+};
+
+struct EventStreamContextData {
+  std::string WatchedPath;
+  std::function, bool)> Receiver;
+
+  EventStreamContextData(
+  std::string &,
+  std::function, bool)>
+  Receiver)
+  : WatchedPath(std::move(WatchedPath)), Receiver(Receiver) {}
+
+  // Needed for FSEvents
+  static void dispose(const void *ctx) {
+delete static_cast(ctx);
+  }
+};
+} // namespace
+
+constexpr const FSEventStreamEventFlags StreamInvalidatingFlags =
+kFSEventStreamEventFlagUserDropped | kFSEventStreamEventFlagKernelDropped |
+kFSEventStreamEventFlagMustScanSubDirs;
+
+constexpr const FSEventStreamEventFlags ModifyingFileEvents =
+kFSEventStreamEventFlagItemCreated | kFSEventStreamEventFlagItemRenamed |
+kFSEventStreamEventFlagItemModified;
+
+static void eventStreamCallback(ConstFSEventStreamRef Stream,
+void *ClientCallBackInfo, size_t NumEvents,
+void *EventPaths,
+const FSEventStreamEventFlags EventFlags[],
+const FSEventStreamEventId EventIds[]) {
+  auto *ctx = static_cast(ClientCallBackInfo);
+
+  std::vector Events;
+  for (size_t i = 0; i < NumEvents; ++i) {
+StringRef Path = ((const char **)EventPaths)[i];
+const FSEventStreamEventFlags Flags = EventFlags[i];
+
+if (Flags & StreamInvalidatingFlags) {
+  Events.emplace_back(DirectoryWatcher::Event{
+  DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""});
+  break;
+} else if (!(Flags & kFSEventStreamEventFlagItemIsFile)) {
+  // Subdirectories aren't supported - if some directory got removed it
+  // must've been the watched directory itself.
+  if ((Flags & kFSEventStreamEventFlagItemRemoved) &&
+  Path == ctx->WatchedPath) {
+Events.emplace_back(DirectoryWatcher::Event{
+DirectoryWatcher::Event::EventKind::WatchedDirRemoved, ""});
+Events.emplace_back(DirectoryWatcher::Event{
+DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""});
+break;
+  }
+   

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 202833.
jkorous added a comment.

linux implementation

- factory method for SemaphorePipe
- *_CLOEXEC flags


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,426 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+std::error_code UniqDirRes = createUniqueDirectory("dirwatcher", pathBuf);
+assert(!UniqDirRes);
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+std::error_code CreateDirRes = create_directory(TestWatchedDir, false);
+assert(!CreateDirRes);
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+Expected ft = openNativeFileForWrite(getPathInWatched(testFile),
+ CD_CreateNew, OF_None);
+if (ft) {
+  closeFile(*ft);
+} else {
+  llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+  llvm::errs() << getPathInWatched(testFile) << "\n";
+  llvm_unreachable("Couldn't create test file.");
+}
+  }
+
+  void deleteFile(const std::string ) {
+std::error_code EC =
+remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+ASSERT_FALSE(EC);
+  }
+};
+
+std::string eventKindToString(const DirectoryWatcher::Event::EventKind K) {
+  switch (K) {
+  case DirectoryWatcher::Event::EventKind::Removed:
+return "Removed";
+  case DirectoryWatcher::Event::EventKind::Modified:
+return "Modified";
+  case DirectoryWatcher::Event::EventKind::WatchedDirRemoved:
+return "WatchedDirRemoved";
+  case DirectoryWatcher::Event::EventKind::WatcherGotInvalidated:
+return "WatcherGotInvalidated";
+  }
+  llvm_unreachable("unknown event kind");
+}
+
+struct VerifyingConsumer {
+  std::vector ExpectedInitial;
+  std::vector ExpectedNonInitial;
+  std::vector OptionalNonInitial;
+  std::vector UnexpectedInitial;
+  std::vector UnexpectedNonInitial;
+  std::mutex Mtx;
+  std::condition_variable ResultIsReady;
+
+  VerifyingConsumer(
+  const std::vector ,
+  const std::vector ,
+  const std::vector  = {})
+  : ExpectedInitial(ExpectedInitial),
+ExpectedNonInitial(ExpectedNonInitial),
+OptionalNonInitial(OptionalNonInitial) {}
+
+  // This method is used by DirectoryWatcher.
+  void consume(DirectoryWatcher::Event E, bool IsInitial) {
+if (IsInitial)
+  consumeInitial(E);
+else
+  consumeNonInitial(E);
+  }
+
+  void consumeInitial(DirectoryWatcher::Event E) {
+std::unique_lock L(Mtx);
+auto It = std::find(ExpectedInitial.begin(), ExpectedInitial.end(), E);
+if (It == ExpectedInitial.end()) {
+  UnexpectedInitial.push_back(E);
+} else {
+  ExpectedInitial.erase(It);
+}
+if (result())
+  ResultIsReady.notify_one();
+  }
+
+  void consumeNonInitial(DirectoryWatcher::Event E) {
+

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 3 inline comments as done.
jkorous added a comment.

I fixed the rest.

There are still some questions you raised that I just responded to and kept 
them as not Done. Feel free to take a look. If nothing comes up I'll commit 
this on Wednesday.




Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40
+// FDRead.
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {

gribozavr wrote:
> jkorous wrote:
> > gribozavr wrote:
> > > gribozavr wrote:
> > > > Three slashes for doc comments?
> > > Don't write what something is used for currently, such comments go out of 
> > > date quickly.  (Just delete the last sentence.)
> > Sure, no problem.
> > 
> > I naturally tend to err on the side of over-commenting as I think I'd 
> > appreciate it myself if I had to understand the code without prior 
> > knowledge - not saying it's intentional or that I have a strong reason to 
> > do that though. You seem to have a strong preference for not having 
> > out-of-date comments with low-ish information value. Just out of curiosity 
> > - is it based on any particular reason or experience?
> > is it based on any particular reason or experience?
> 
> Yes, primarily working on the Swift compiler and the standard library, when 
> everything was changing very quickly.  My current work also confirms the same 
> -- a lot of time when I see a comment about the "current usage" or other 
> incidental information that is not the contract of the API, it tends to be 
> outdated.  Approximately nobody will change such a comment when another user 
> is added ("I'm just reusing the code..."), even when the quoted user already 
> became non-representative of the usage pattern, or the usage pattern has 
> changed.  However, when changing the API contract people typically do change 
> the comment.
> 
> It also makes sense to me in abstract: reading that X happens to be used for 
> Y does not necessarily help understand X better -- it is only a 
> cross-reference that I could find myself with an IDE command; I still need to 
> understand the design of Y and the interaction with X, and then using my past 
> experience infer what X was intended to be.
> 
> Saying that X is intended to be used only by Y is a different story of 
> course, that's design documentation.
> 
> Providing an example of usage is also fine, but it should be phrased as an 
> example that can't become stale.
Thank you.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:52
+close(FDWrite);
+close(FDRead);
+  }

gribozavr wrote:
> jkorous wrote:
> > gribozavr wrote:
> > > Since it closes the file descriptors in the destructor, I feel like it 
> > > should also be responsible for calling `pipe` in the constructor.
> > I know what you mean - my "oop feel" was telling me it's wrong too. It's 
> > not just the pipe in this class but also the inotify descriptor in the 
> > watcher class.
> > 
> > The problem is that the only reasonable way how to communicate failures 
> > from constructors that I am aware of are exceptions which we don't use in 
> > llvm. That's why I moved most of the stuff that can fail even before any 
> > work is actually started to the factory method (with the exception of epoll 
> > file descriptor as that felt like making its scope unnecessarily larger).
> > 
> > Thinking about it now, I am starting to doubt that it makes life any easier 
> > for client code as it still has to cope with failure communicated as 
> > WatcherGotInvalidated event via receiver.
> > 
> > What do you think?
> You could add a factory function to SemaphorePipe, but... I feel like trying 
> to recover from a failure in the pipe() call is a bit like trying to recover 
> from a memory allocation failure.  The process is probably hitting a file 
> descriptor limit or something like that, and is likely going to fail anyway.  
> I'd probably trigger a fatal error if pipe() fails.
> 
> This class is a lot like pthread_mutex_init -- it can fail "gracefully", but 
> there's no way for the caller to recover -- the caller needs a mutex to 
> proceed.
I see what you mean and mostly agree. Anyways, let's allow clients to handle 
such funny moments themselves as much as they can.

I'll factor out the pipe call to the factory method.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:317
+
+  if (pipe(InotifyPollingStopperFDs) == -1)
+return nullptr;

gribozavr wrote:
> Use pipe2() with O_CLOEXEC, to avoid leaking the file descriptors to child 
> processes?
You're right, I didn't account for this. Added the flag also to 
`inotify_init1()` and `epoll_create1()` calls.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:317
+
+  if (pipe(InotifyPollingStopperFDs) == -1)
+return nullptr;

Use pipe2() with O_CLOEXEC, to avoid leaking the file descriptors to child 
processes?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40
+// FDRead.
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {

jkorous wrote:
> gribozavr wrote:
> > gribozavr wrote:
> > > Three slashes for doc comments?
> > Don't write what something is used for currently, such comments go out of 
> > date quickly.  (Just delete the last sentence.)
> Sure, no problem.
> 
> I naturally tend to err on the side of over-commenting as I think I'd 
> appreciate it myself if I had to understand the code without prior knowledge 
> - not saying it's intentional or that I have a strong reason to do that 
> though. You seem to have a strong preference for not having out-of-date 
> comments with low-ish information value. Just out of curiosity - is it based 
> on any particular reason or experience?
> is it based on any particular reason or experience?

Yes, primarily working on the Swift compiler and the standard library, when 
everything was changing very quickly.  My current work also confirms the same 
-- a lot of time when I see a comment about the "current usage" or other 
incidental information that is not the contract of the API, it tends to be 
outdated.  Approximately nobody will change such a comment when another user is 
added ("I'm just reusing the code..."), even when the quoted user already 
became non-representative of the usage pattern, or the usage pattern has 
changed.  However, when changing the API contract people typically do change 
the comment.

It also makes sense to me in abstract: reading that X happens to be used for Y 
does not necessarily help understand X better -- it is only a cross-reference 
that I could find myself with an IDE command; I still need to understand the 
design of Y and the interaction with X, and then using my past experience infer 
what X was intended to be.

Saying that X is intended to be used only by Y is a different story of course, 
that's design documentation.

Providing an example of usage is also fine, but it should be phrased as an 
example that can't become stale.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:52
+close(FDWrite);
+close(FDRead);
+  }

jkorous wrote:
> gribozavr wrote:
> > Since it closes the file descriptors in the destructor, I feel like it 
> > should also be responsible for calling `pipe` in the constructor.
> I know what you mean - my "oop feel" was telling me it's wrong too. It's not 
> just the pipe in this class but also the inotify descriptor in the watcher 
> class.
> 
> The problem is that the only reasonable way how to communicate failures from 
> constructors that I am aware of are exceptions which we don't use in llvm. 
> That's why I moved most of the stuff that can fail even before any work is 
> actually started to the factory method (with the exception of epoll file 
> descriptor as that felt like making its scope unnecessarily larger).
> 
> Thinking about it now, I am starting to doubt that it makes life any easier 
> for client code as it still has to cope with failure communicated as 
> WatcherGotInvalidated event via receiver.
> 
> What do you think?
You could add a factory function to SemaphorePipe, but... I feel like trying to 
recover from a failure in the pipe() call is a bit like trying to recover from 
a memory allocation failure.  The process is probably hitting a file descriptor 
limit or something like that, and is likely going to fail anyway.  I'd probably 
trigger a fatal error if pipe() fails.

This class is a lot like pthread_mutex_init -- it can fail "gracefully", but 
there's no way for the caller to recover -- the caller needs a mutex to proceed.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 202467.
jkorous marked 2 inline comments as done.
jkorous added a comment.

Addressed comments.


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,426 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+std::error_code UniqDirRes = createUniqueDirectory("dirwatcher", pathBuf);
+assert(!UniqDirRes);
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+std::error_code CreateDirRes = create_directory(TestWatchedDir, false);
+assert(!CreateDirRes);
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+Expected ft = openNativeFileForWrite(getPathInWatched(testFile),
+ CD_CreateNew, OF_None);
+if (ft) {
+  closeFile(*ft);
+} else {
+  llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+  llvm::errs() << getPathInWatched(testFile) << "\n";
+  llvm_unreachable("Couldn't create test file.");
+}
+  }
+
+  void deleteFile(const std::string ) {
+std::error_code EC =
+remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+ASSERT_FALSE(EC);
+  }
+};
+
+std::string eventKindToString(const DirectoryWatcher::Event::EventKind K) {
+  switch (K) {
+  case DirectoryWatcher::Event::EventKind::Removed:
+return "Removed";
+  case DirectoryWatcher::Event::EventKind::Modified:
+return "Modified";
+  case DirectoryWatcher::Event::EventKind::WatchedDirRemoved:
+return "WatchedDirRemoved";
+  case DirectoryWatcher::Event::EventKind::WatcherGotInvalidated:
+return "WatcherGotInvalidated";
+  }
+  llvm_unreachable("unknown event kind");
+}
+
+struct VerifyingConsumer {
+  std::vector ExpectedInitial;
+  std::vector ExpectedNonInitial;
+  std::vector OptionalNonInitial;
+  std::vector UnexpectedInitial;
+  std::vector UnexpectedNonInitial;
+  std::mutex Mtx;
+  std::condition_variable ResultIsReady;
+
+  VerifyingConsumer(
+  const std::vector ,
+  const std::vector ,
+  const std::vector  = {})
+  : ExpectedInitial(ExpectedInitial),
+ExpectedNonInitial(ExpectedNonInitial),
+OptionalNonInitial(OptionalNonInitial) {}
+
+  // This method is used by DirectoryWatcher.
+  void consume(DirectoryWatcher::Event E, bool IsInitial) {
+if (IsInitial)
+  consumeInitial(E);
+else
+  consumeNonInitial(E);
+  }
+
+  void consumeInitial(DirectoryWatcher::Event E) {
+std::unique_lock L(Mtx);
+auto It = std::find(ExpectedInitial.begin(), ExpectedInitial.end(), E);
+if (It == ExpectedInitial.end()) {
+  UnexpectedInitial.push_back(E);
+} else {
+  ExpectedInitial.erase(It);
+}
+if (result())
+  ResultIsReady.notify_one();
+  }
+
+  void consumeNonInitial(DirectoryWatcher::Event E) {
+std::unique_lock L(Mtx);
+ 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 20 inline comments as done.
jkorous added inline comments.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40
+// FDRead.
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {

gribozavr wrote:
> gribozavr wrote:
> > Three slashes for doc comments?
> Don't write what something is used for currently, such comments go out of 
> date quickly.  (Just delete the last sentence.)
Sure, no problem.

I naturally tend to err on the side of over-commenting as I think I'd 
appreciate it myself if I had to understand the code without prior knowledge - 
not saying it's intentional or that I have a strong reason to do that though. 
You seem to have a strong preference for not having out-of-date comments with 
low-ish information value. Just out of curiosity - is it based on any 
particular reason or experience?



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:52
+close(FDWrite);
+close(FDRead);
+  }

gribozavr wrote:
> Since it closes the file descriptors in the destructor, I feel like it should 
> also be responsible for calling `pipe` in the constructor.
I know what you mean - my "oop feel" was telling me it's wrong too. It's not 
just the pipe in this class but also the inotify descriptor in the watcher 
class.

The problem is that the only reasonable way how to communicate failures from 
constructors that I am aware of are exceptions which we don't use in llvm. 
That's why I moved most of the stuff that can fail even before any work is 
actually started to the factory method (with the exception of epoll file 
descriptor as that felt like making its scope unnecessarily larger).

Thinking about it now, I am starting to doubt that it makes life any easier for 
client code as it still has to cope with failure communicated as 
WatcherGotInvalidated event via receiver.

What do you think?



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:153
+  // Not locking - caller has to lock Mtx
+  llvm::Optional Result() const {
+if (ExpectedInitial.empty() && ExpectedNonInitial.empty() &&

gribozavr wrote:
> Please name functions consistently -- there's both `consume()` that starts 
> with lowercase, and `Result()` that starts with uppercase.
> 
> Please refer to the current naming rules in the style guide and apply 
> everywhere in the patch.
> 
> 
Sorry about that. This is definitely my weak point.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Very nice testing approach!




Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:20
+/// Provides notifications for file changes in a directory.
+
+/// Invokes client-provided function on every filesystem event in the watched

Looks like triple slashes on empty lines got removed, splitting the doc comment.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40
+// FDRead.
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {

Three slashes for doc comments?



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40
+// FDRead.
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {

gribozavr wrote:
> Three slashes for doc comments?
Don't write what something is used for currently, such comments go out of date 
quickly.  (Just delete the last sentence.)



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:41
+// Currently used just for one-off termination signal.
+struct SemaphorPipe {
+  // Expecting two file-descriptors opened as a pipe in the canonical POSIX

Semaphor*e*



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:42
+struct SemaphorPipe {
+  // Expecting two file-descriptors opened as a pipe in the canonical POSIX
+  // order: pipefd[0] refers to the read end of the pipe. pipefd[1] refers to

"Expects"

Three slashes for doc comments.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:49
+assert(Result != -1);
+  };
+  ~SemaphorPipe() {

Extra semicolon.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:52
+close(FDWrite);
+close(FDRead);
+  }

Since it closes the file descriptors in the destructor, I feel like it should 
also be responsible for calling `pipe` in the constructor.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:58
+
+// Mutex-protected queue of Events.
+class EventQueue {

Three slashes for doc comments.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:120
+
+  // Consuming inotify events and pushing events to the Queue.
+  void InotifyPollingLoop();

"consumes", "pushes"



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:109
+
+  // This method is used by DirectoryWatcher
+  void consume(DirectoryWatcher::Event E, bool IsInitial) {

Please add a period at the end of the comment (everywhere in the patch).



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:153
+  // Not locking - caller has to lock Mtx
+  llvm::Optional Result() const {
+if (ExpectedInitial.empty() && ExpectedNonInitial.empty() &&

Please name functions consistently -- there's both `consume()` that starts with 
lowercase, and `Result()` that starts with uppercase.

Please refer to the current naming rules in the style guide and apply 
everywhere in the patch.





Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:210
+
+  // TestConsumer didn't reach the expected state in given time.
+  EXPECT_TRUE(WaitForExpectedStateResult.wait_for(std::chrono::seconds(3)) ==

"If the following assertions fail, it is a sign that ..."

Also you can stream the message into the EXPECT_TRUE, it will be printed if the 
assertion fails.

EXPECT_TRUE(...) << "whatever";



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:224
+
+TEST(DirectoryWatcherTest, initialScanSync) {
+  DirectoryWatcherTestFixture fixture;

Test names start with an uppercase letter (`InitialScanSync`).  Please apply 
everywhere in the patch.



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:243
+  },
+  true);
+

Add /*waitForInitialSync=*/ ?  (everywhere in the patch)



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:304
+  {
+  {DirectoryWatcher::Event::EventKind::Modified, "a"},
+  }};

Delete the comma and wrap onto one line.



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:419
+
+std::error_code setTimeRes = 
llvm::sys::fs::setLastAccessAndModificationTime(FD, NewTimePt,
+NewTimePt);

80 columns.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

One more thing.

On macOS FSEvents are coalesced more or less at will and it became quite 
apparent when I was creating automatic tests - I was for example receiving 
coalesced events Added & Modified & Removed. We had a discussion about how to 
deal with this and it turned out that the existing client doesn't actually care 
whether a file was created or whether it was modified. I believe that by 
removing the EventKind::Added we still keep the interface sane while removing 
the ambiguity.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 201744.
jkorous added a comment.

Remove DirectoryWatcher::Event::EventKind::Added


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,425 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+std::error_code UniqDirRes = createUniqueDirectory("dirwatcher", pathBuf);
+assert(!UniqDirRes);
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+std::error_code CreateDirRes = create_directory(TestWatchedDir, false);
+assert(!CreateDirRes);
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+Expected ft = openNativeFileForWrite(getPathInWatched(testFile),
+ CD_CreateNew, OF_None);
+if (ft) {
+  closeFile(*ft);
+} else {
+  llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+  llvm::errs() << getPathInWatched(testFile) << "\n";
+  llvm_unreachable("Couldn't create test file.");
+}
+  }
+
+  void deleteFile(const std::string ) {
+std::error_code EC =
+remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+ASSERT_FALSE(EC);
+  }
+};
+
+std::string EventKindToString(const DirectoryWatcher::Event::EventKind K) {
+  switch (K) {
+  case DirectoryWatcher::Event::EventKind::Removed:
+return "Removed";
+  case DirectoryWatcher::Event::EventKind::Modified:
+return "Modified";
+  case DirectoryWatcher::Event::EventKind::WatchedDirRemoved:
+return "WatchedDirRemoved";
+  case DirectoryWatcher::Event::EventKind::WatcherGotInvalidated:
+return "WatcherGotInvalidated";
+  }
+  llvm_unreachable("unknown event kind");
+}
+
+struct VerifyingConsumer {
+  std::vector ExpectedInitial;
+  std::vector ExpectedNonInitial;
+  std::vector OptionalNonInitial;
+  std::vector UnexpectedInitial;
+  std::vector UnexpectedNonInitial;
+  std::mutex Mtx;
+  std::condition_variable ResultIsReady;
+
+  VerifyingConsumer(
+  const std::vector ,
+  const std::vector ,
+  const std::vector  = {})
+  : ExpectedInitial(ExpectedInitial),
+ExpectedNonInitial(ExpectedNonInitial),
+OptionalNonInitial(OptionalNonInitial) {}
+
+  // This method is used by DirectoryWatcher
+  void consume(DirectoryWatcher::Event E, bool IsInitial) {
+if (IsInitial)
+  consumeInitial(E);
+else
+  consumeNonInitial(E);
+  }
+
+  void consumeInitial(DirectoryWatcher::Event E) {
+std::unique_lock L(Mtx);
+auto It = std::find(ExpectedInitial.begin(), ExpectedInitial.end(), E);
+if (It == ExpectedInitial.end()) {
+  UnexpectedInitial.push_back(E);
+} else {
+  ExpectedInitial.erase(It);
+}
+if (Result())
+  ResultIsReady.notify_one();
+  }
+
+  void consumeNonInitial(DirectoryWatcher::Event E) {
+std::unique_lock L(Mtx);
+auto It = 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 201338.
jkorous added a comment.

- simplify link libraries in cmake
- fix Release build (messed-up asserts)


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,427 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+std::error_code UniqDirRes = createUniqueDirectory("dirwatcher", pathBuf);
+assert(!UniqDirRes);
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+std::error_code CreateDirRes = create_directory(TestWatchedDir, false);
+assert(!CreateDirRes);
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+Expected ft = openNativeFileForWrite(getPathInWatched(testFile),
+ CD_CreateNew, OF_None);
+if (ft) {
+  closeFile(*ft);
+} else {
+  llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+  llvm::errs() << getPathInWatched(testFile) << "\n";
+  llvm_unreachable("Couldn't create test file.");
+}
+  }
+
+  void deleteFile(const std::string ) {
+std::error_code EC =
+remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+ASSERT_FALSE(EC);
+  }
+};
+
+std::string EventKindToString(const DirectoryWatcher::Event::EventKind K) {
+  switch (K) {
+  case DirectoryWatcher::Event::EventKind::Added:
+return "Added";
+  case DirectoryWatcher::Event::EventKind::Removed:
+return "Removed";
+  case DirectoryWatcher::Event::EventKind::Modified:
+return "Modified";
+  case DirectoryWatcher::Event::EventKind::WatchedDirRemoved:
+return "WatchedDirRemoved";
+  case DirectoryWatcher::Event::EventKind::WatcherGotInvalidated:
+return "WatcherGotInvalidated";
+  }
+  llvm_unreachable("unknown event kind");
+}
+
+struct VerifyingConsumer {
+  std::vector ExpectedInitial;
+  std::vector ExpectedNonInitial;
+  std::vector OptionalNonInitial;
+  std::vector UnexpectedInitial;
+  std::vector UnexpectedNonInitial;
+  std::mutex Mtx;
+  std::condition_variable ResultIsReady;
+
+  VerifyingConsumer(
+  const std::vector ,
+  const std::vector ,
+  const std::vector  = {})
+  : ExpectedInitial(ExpectedInitial),
+ExpectedNonInitial(ExpectedNonInitial),
+OptionalNonInitial(OptionalNonInitial) {}
+
+  // This method is used by DirectoryWatcher
+  void consume(DirectoryWatcher::Event E, bool IsInitial) {
+if (IsInitial)
+  consumeInitial(E);
+else
+  consumeNonInitial(E);
+  }
+
+  void consumeInitial(DirectoryWatcher::Event E) {
+std::unique_lock L(Mtx);
+auto It = std::find(ExpectedInitial.begin(), ExpectedInitial.end(), E);
+if (It == ExpectedInitial.end()) {
+  UnexpectedInitial.push_back(E);
+} else {
+  ExpectedInitial.erase(It);
+}
+if (Result())
+  ResultIsReady.notify_one();
+  }
+
+  

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 201321.
jkorous added a comment.

Specify what "file modified" means and add a test for metadata change


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

https://reviews.llvm.org/D58418

Files:
  clang/cmake/modules/AddClang.cmake
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,424 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+assert(!createUniqueDirectory("dirwatcher", pathBuf));
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+assert(!create_directory(TestWatchedDir, false));
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+Expected ft = openNativeFileForWrite(getPathInWatched(testFile),
+ CD_CreateNew, OF_None);
+if (ft) {
+  closeFile(*ft);
+} else {
+  llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+  llvm::errs() << getPathInWatched(testFile) << "\n";
+  llvm_unreachable("Couldn't create test file.");
+}
+  }
+
+  void deleteFile(const std::string ) {
+std::error_code EC =
+remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+ASSERT_FALSE(EC);
+  }
+};
+
+std::string EventKindToString(const DirectoryWatcher::Event::EventKind K) {
+  switch (K) {
+  case DirectoryWatcher::Event::EventKind::Added:
+return "Added";
+  case DirectoryWatcher::Event::EventKind::Removed:
+return "Removed";
+  case DirectoryWatcher::Event::EventKind::Modified:
+return "Modified";
+  case DirectoryWatcher::Event::EventKind::WatchedDirRemoved:
+return "WatchedDirRemoved";
+  case DirectoryWatcher::Event::EventKind::WatcherGotInvalidated:
+return "WatcherGotInvalidated";
+  }
+  llvm_unreachable("unknown event kind");
+}
+
+struct VerifyingConsumer {
+  std::vector ExpectedInitial;
+  std::vector ExpectedNonInitial;
+  std::vector OptionalNonInitial;
+  std::vector UnexpectedInitial;
+  std::vector UnexpectedNonInitial;
+  std::mutex Mtx;
+  std::condition_variable ResultIsReady;
+
+  VerifyingConsumer(
+  const std::vector ,
+  const std::vector ,
+  const std::vector  = {})
+  : ExpectedInitial(ExpectedInitial),
+ExpectedNonInitial(ExpectedNonInitial),
+OptionalNonInitial(OptionalNonInitial) {}
+
+  // This method is used by DirectoryWatcher
+  void consume(DirectoryWatcher::Event E, bool IsInitial) {
+if (IsInitial)
+  consumeInitial(E);
+else
+  consumeNonInitial(E);
+  }
+
+  void consumeInitial(DirectoryWatcher::Event E) {
+std::unique_lock L(Mtx);
+auto It = std::find(ExpectedInitial.begin(), ExpectedInitial.end(), E);
+if (It == ExpectedInitial.end()) {
+  UnexpectedInitial.push_back(E);
+} else {
+  ExpectedInitial.erase(It);
+}
+if (Result())
+  ResultIsReady.notify_one();
+  }
+
+  void consumeNonInitial(DirectoryWatcher::Event E) {
+

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 6 inline comments as done.
jkorous added inline comments.



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:131
+  // the async event handling picks them up. Can make this test flaky.
+  std::this_thread::sleep_for(std::chrono::milliseconds(100)); // 0.1s
+}

jkorous wrote:
> gribozavr wrote:
> > I'm certain this sleep will be flaky on heavily-loaded CI machines.  If you 
> > are going to leave it as a sleep, please make it 1s.  But is there really 
> > no better way?
> That was exactly my thinking! Honestly, I don't know - I wasn't able to come 
> up with any reasonably simple, deterministic approach even on a single 
> platform :(
> I eventually picked 0.1s as a tradeoff between slowing the test for everyone 
> and getting less false positives.
> 
> The problem as I understand it is that we're making changes and monitoring 
> them asynchronously with no guarantee from the kernel API (true for FSEvents, 
> not 100% about inotify) about when (if) we receive notifications.
> 
> If you have any idea for robust testing approach I'd be totally happy to use 
> it.
I found a way how to use more generous timeout without slowing down the test 
for every run - inspired by Argyrios' idea about using different thread + 
semaphore.

I am using 3 seconds for now. If that's not enough, just let me know.



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:357
+
+  EXPECT_TRUE(eventConsumer.AreExpectedPresentInNonInitial(
+  {{DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""}}));

gribozavr wrote:
> I would strongly prefer if you used the gmock matchers (like Contains); as 
> written, when the test fails, the only error we would get would be like 
> "expected: true, actual: false".
Since the tests are using are based on something like "eventual correctness" 
instead of one-time check I didn't use gmock matchers but implemented some 
custom diagnostics.

Example of the failed test:
```
/Users/jankorous/src/llvm-monorepo/llvm-project/clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:185:
 Failure
Value of: *TestConsumer.Result()
  Actual: false
Expected: true
Expected but not seen non-initial events:
Removed a
Unexpected non-initial events seen:
Added a
```


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 201114.
jkorous added a comment.

Reimplemented tests with std::futures which allowed to use more generous 
timeout while not slowing down the happy paths.


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

https://reviews.llvm.org/D58418

Files:
  clang/cmake/modules/AddClang.cmake
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,362 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+assert(!createUniqueDirectory("dirwatcher", pathBuf));
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+assert(!create_directory(TestWatchedDir, false));
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+Expected ft = openNativeFileForWrite(getPathInWatched(testFile),
+ CD_CreateNew, OF_None);
+if (ft) {
+  closeFile(*ft);
+} else {
+  llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+  llvm::errs() << getPathInWatched(testFile) << "\n";
+  llvm_unreachable("Couldn't create test file.");
+}
+  }
+
+  void deleteFile(const std::string ) {
+std::error_code EC =
+remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+ASSERT_FALSE(EC);
+  }
+};
+
+std::string EventKindToString(const DirectoryWatcher::Event::EventKind K) {
+  switch (K) {
+  case DirectoryWatcher::Event::EventKind::Added:
+return "Added";
+  case DirectoryWatcher::Event::EventKind::Removed:
+return "Removed";
+  case DirectoryWatcher::Event::EventKind::Modified:
+return "Modified";
+  case DirectoryWatcher::Event::EventKind::WatchedDirRemoved:
+return "WatchedDirRemoved";
+  case DirectoryWatcher::Event::EventKind::WatcherGotInvalidated:
+return "WatcherGotInvalidated";
+  }
+  llvm_unreachable("unknown event kind");
+}
+
+struct VerifyingConsumer {
+  std::vector ExpectedInitial;
+  std::vector ExpectedNonInitial;
+  std::vector UnexpectedInitial;
+  std::vector UnexpectedNonInitial;
+  std::mutex Mtx;
+  std::condition_variable ResultIsReady;
+
+  VerifyingConsumer(
+  const std::vector ,
+  const std::vector )
+  : ExpectedInitial(ExpectedInitial),
+ExpectedNonInitial(ExpectedNonInitial) {}
+
+  // This method is used by DirectoryWatcher
+  void consume(DirectoryWatcher::Event E, bool IsInitial) {
+std::unique_lock L(Mtx);
+std::vector  =
+IsInitial ? ExpectedInitial : ExpectedNonInitial;
+std::vector  =
+IsInitial ? UnexpectedInitial : UnexpectedNonInitial;
+auto It =
+std::find(WhichExpectedVector.begin(), WhichExpectedVector.end(), E);
+if (It == WhichExpectedVector.end()) {
+  WhichUnexpectedVector.push_back(E);
+} else {
+  WhichExpectedVector.erase(It);
+}
+if (Result())
+  ResultIsReady.notify_one();
+  }
+
+  // This method is used by DirectoryWatcher
+  void consume(llvm::ArrayRef Es, 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 200811.

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

https://reviews.llvm.org/D58418

Files:
  clang/cmake/modules/AddClang.cmake
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,346 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+// Intentionally trivial - expecting just a very small numbers of events.
+class TestEventConsumer {
+  std::vector InitialEvents;
+  std::vector NonInitialEvents;
+  sys::Mutex Mtx;
+
+  // Expecting only very small sets - don't bother with anything smart.
+  static bool
+  AreExpectedPresent(const std::vector ,
+ const std::vector ) {
+for (const auto  : expected) {
+  if (std::find(actual.begin(), actual.end(), e) == actual.end())
+return false;
+}
+return true;
+  }
+
+public:
+  void push(llvm::ArrayRef Events, bool isInitial) {
+sys::ScopedLock L(Mtx);
+if (isInitial) {
+  for (const auto  : Events)
+InitialEvents.push_back(E);
+} else {
+  for (const auto  : Events)
+NonInitialEvents.push_back(E);
+}
+  }
+
+  std::vector getInitialEvents() {
+sys::ScopedLock L(Mtx);
+return InitialEvents;
+  }
+  std::vector getNonInitialEvents() {
+sys::ScopedLock L(Mtx);
+return NonInitialEvents;
+  }
+
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInInitial(
+  const std::vector ) {
+sys::ScopedLock L(Mtx);
+return AreExpectedPresent(InitialEvents, expected);
+  }
+
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInNonInitial(
+  const std::vector ) {
+sys::ScopedLock L(Mtx);
+return AreExpectedPresent(NonInitialEvents, expected);
+  }
+};
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+assert(!createUniqueDirectory("dirwatcher", pathBuf));
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+assert(!create_directory(TestWatchedDir, false));
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+Expected ft = openNativeFileForWrite(getPathInWatched(testFile),
+ CD_CreateNew, OF_None);
+if (ft) {
+  closeFile(*ft);
+} else {
+  llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+  llvm::errs() << getPathInWatched(testFile) << "\n";
+  llvm_unreachable("Couldn't create test file.");
+}
+  }
+
+  void deleteFile(const std::string ) {
+std::error_code EC =
+remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+ASSERT_FALSE(EC);
+  }
+};
+
+void WaitForAsync() {
+  // This is just a heuristic - trying to wait for changes to FS to propagate so
+  // the async event handling picks them up. Can make this test flaky.
+  std::this_thread::sleep_for(std::chrono::milliseconds(100)); // 0.1s
+}
+
+} // namespace
+
+TEST(DirectoryWatcherTest, initialScanSync) {
+  DirectoryWatcherTestFixture fixture;
+  TestEventConsumer eventConsumer;
+
+  

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 200810.
jkorous marked an inline comment as done.
jkorous added a comment.

Remove busy waits.


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

https://reviews.llvm.org/D58418

Files:
  clang/cmake/modules/AddClang.cmake
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,346 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+// Intentionally trivial - expecting just a very small numbers of events.
+class TestEventConsumer {
+  std::vector InitialEvents;
+  std::vector NonInitialEvents;
+  sys::Mutex Mtx;
+
+  // Expecting only very small sets - don't bother with anything smart.
+  static bool
+  AreExpectedPresent(const std::vector ,
+ const std::vector ) {
+for (const auto  : expected) {
+  if (std::find(actual.begin(), actual.end(), e) == actual.end())
+return false;
+}
+return true;
+  }
+
+public:
+  void push(llvm::ArrayRef Events, bool isInitial) {
+sys::ScopedLock L(Mtx);
+if (isInitial) {
+  for (const auto  : Events)
+InitialEvents.push_back(E);
+} else {
+  for (const auto  : Events)
+NonInitialEvents.push_back(E);
+}
+  }
+
+  std::vector getInitialEvents() {
+sys::ScopedLock L(Mtx);
+return InitialEvents;
+  }
+  std::vector getNonInitialEvents() {
+sys::ScopedLock L(Mtx);
+return NonInitialEvents;
+  }
+
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInInitial(
+  const std::vector ) {
+sys::ScopedLock L(Mtx);
+return AreExpectedPresent(InitialEvents, expected);
+  }
+
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInNonInitial(
+  const std::vector ) {
+sys::ScopedLock L(Mtx);
+return AreExpectedPresent(NonInitialEvents, expected);
+  }
+};
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+assert(!createUniqueDirectory("dirwatcher", pathBuf));
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+assert(!create_directory(TestWatchedDir, false));
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+Expected ft = openNativeFileForWrite(getPathInWatched(testFile),
+ CD_CreateNew, OF_None);
+if (ft) {
+  closeFile(*ft);
+} else {
+  llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+  llvm::errs() << getPathInWatched(testFile) << "\n";
+  llvm_unreachable("Couldn't create test file.");
+}
+  }
+
+  void deleteFile(const std::string ) {
+std::error_code EC =
+remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+ASSERT_FALSE(EC);
+  }
+};
+
+void WaitForAsync() {
+  // This is just a heuristic - trying to wait for changes to FS to propagate so
+  // the async event handling picks them up. Can make this test flaky.
+  std::this_thread::sleep_for(std::chrono::milliseconds(100)); // 0.1s
+}
+
+} // namespace
+
+TEST(DirectoryWatcherTest, 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 9 inline comments as done.
jkorous added a comment.

Thanks for taking a look Kadir!
After yesterday's discussion with Dmitri I removed all those busy waits. Seems 
like the code is not much more complex now. I am going to update the diff and 
off to fixing the tests.




Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:211
+  // ... inotify-originated events processing ever after.
+  while (true) {
+bool GotEvent = false;

kadircet wrote:
> kadircet wrote:
> > `while (!Stop)` ?
> why the loop always tries to empty the queue? what would be broken if we 
> simply stopped if `Stop` was set?
> 
> Also if that is important current implementation doesn't seem to be achiving 
> that, since some events can be pushed to the queue after while loop exits.
I changed the design - stopping condition for this loop is now finding 
`WatcherGotInvalidated` in the queue itself.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:48
+  return llvm::None;
+} else {
+  DirectoryWatcher::Event Front = Events.front();

nit: get rid of the else



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:156
+
+for (char *p = Buf; p < Buf + NumRead;) {
+  if (p + sizeof(struct inotify_event) > Buf + NumRead) {

naming, it should be capital `P`



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:211
+  // ... inotify-originated events processing ever after.
+  while (true) {
+bool GotEvent = false;

`while (!Stop)` ?



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:211
+  // ... inotify-originated events processing ever after.
+  while (true) {
+bool GotEvent = false;

kadircet wrote:
> `while (!Stop)` ?
why the loop always tries to empty the queue? what would be broken if we simply 
stopped if `Stop` was set?

Also if that is important current implementation doesn't seem to be achiving 
that, since some events can be pushed to the queue after while loop exits.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:223
+break;
+  std::this_thread::sleep_for(std::chrono::milliseconds(1));
+}

maybe add a condition variable for queue state and waint on it?



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:244
+  ReceivingThread([this]() { EventReceivingLoop(); }) {
+  while (WaitForInitialSync && !Stop && !FinishedInitScan) {
+std::this_thread::sleep_for(std::chrono::milliseconds(1));

why not use two condition variables for stop and finishedinitscan?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:136
+
+const int PollResult = poll(, 1, TimeoutMs);
+// There are inotify events waiting to be read!

jkorous wrote:
> gribozavr wrote:
> > What is the role of the timeout and why does it need to be so small?
> The whole idea is that we can't block on `read()` if we ever want to stop 
> watching the directory, release resources (file descriptors, threads) and 
> correctly destruct the DirectoryWatcher instance either
> - because of a bug in some other thread in the implementation
> - or asynchronous client action (e. g. destructor being called) in main 
> application thread
> 
> The timeout adds latency in those scenarios.
Waking up 1000 times a second is not great -- it will lead to battery drain on 
laptops etc.

Please see 
https://stackoverflow.com/questions/8593004/waiting-on-a-condition-pthread-cond-wait-and-a-socket-change-select-simultan
 for non-busy-wait options.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:111
+
+  std::unique_ptr>

"alignof()" expression is standard C++ since C++11. (No need for underscores.)



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:114
+  ManagedBuffer = llvm::make_unique>();
+  char *const Buf = ManagedBuffer->buffer;

use 'auto' to store the return value of make_unique?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 200389.
jkorous marked 4 inline comments as done.
jkorous added a comment.

Addressed comments.
Changed semantics of one of std::atomic in linux implementation.


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

https://reviews.llvm.org/D58418

Files:
  clang/cmake/modules/AddClang.cmake
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,346 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+// Intentionally trivial - expecting just a very small numbers of events.
+class TestEventConsumer {
+  std::vector InitialEvents;
+  std::vector NonInitialEvents;
+  sys::Mutex Mtx;
+
+  // Expecting only very small sets - don't bother with anything smart.
+  static bool
+  AreExpectedPresent(const std::vector ,
+ const std::vector ) {
+for (const auto  : expected) {
+  if (std::find(actual.begin(), actual.end(), e) == actual.end())
+return false;
+}
+return true;
+  }
+
+public:
+  void push(llvm::ArrayRef Events, bool isInitial) {
+sys::ScopedLock L(Mtx);
+if (isInitial) {
+  for (const auto  : Events)
+InitialEvents.push_back(E);
+} else {
+  for (const auto  : Events)
+NonInitialEvents.push_back(E);
+}
+  }
+
+  std::vector getInitialEvents() {
+sys::ScopedLock L(Mtx);
+return InitialEvents;
+  }
+  std::vector getNonInitialEvents() {
+sys::ScopedLock L(Mtx);
+return NonInitialEvents;
+  }
+
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInInitial(
+  const std::vector ) {
+sys::ScopedLock L(Mtx);
+return AreExpectedPresent(InitialEvents, expected);
+  }
+
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInNonInitial(
+  const std::vector ) {
+sys::ScopedLock L(Mtx);
+return AreExpectedPresent(NonInitialEvents, expected);
+  }
+};
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+assert(!createUniqueDirectory("dirwatcher", pathBuf));
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+assert(!create_directory(TestWatchedDir, false));
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+Expected ft = openNativeFileForWrite(getPathInWatched(testFile),
+ CD_CreateNew, OF_None);
+if (ft) {
+  closeFile(*ft);
+} else {
+  llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+  llvm::errs() << getPathInWatched(testFile) << "\n";
+  llvm_unreachable("Couldn't create test file.");
+}
+  }
+
+  void deleteFile(const std::string ) {
+std::error_code EC =
+remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+ASSERT_FALSE(EC);
+  }
+};
+
+void WaitForAsync() {
+  // This is just a heuristic - trying to wait for changes to FS to propagate so
+  // the async event handling picks them up. Can make this test flaky.
+  std::this_thread::sleep_for(std::chrono::milliseconds(100)); // 0.1s

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 36 inline comments as done.
jkorous added a comment.

I addressed most of the comments.

What is left:

- rewrite test with gmock matchers
- write test for metadata of a file in watched dir being changed




Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:46
+/// The watched directory got deleted. No more events will follow.
+DirectoryDeleted,
+  };

gribozavr wrote:
> `DirectoryRemoved` (for consistency with `Removed`)
> 
> Also, how about `TopDirectoryRemoved`?  Otherwise it sounds like some random 
> directory was removed.
I used `WatchedDirRemoved`.



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:62
+  bool waitInitialSync,
+  std::string );
+

gribozavr wrote:
> Why not return `llvm::ErrorOr>`?
> 
> I also don't understand why we need unique_ptr.  If you change 
> `Implementation ` to `std::unique_ptr Impl;`, 
> `DirectoryWatcher` becomes a move-only type, and `create` can return 
> `llvm::ErrorOr`.
I personally didn't like the C-like `std::string& error` in the interface. But 
I feel like having a polymorphic class (which doesn't really fit into 
`ErrorOr`) is lesser evil than the original pimpl design.
WDYT?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:95
+EventKind Kind;
+/// Filename - a relative path to the watched directory or empty string in
+/// case event is related to the directory itself.

gribozavr wrote:
> gribozavr wrote:
> > gribozavr wrote:
> > > Don't repeat field names in comments.
> > "a relative path" -- relative to what?
> Is it really a path to the directory?
I reworded the comment.



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:123
+   DirectoryWatcher::EventReceiver Receiver,
+   bool WaitInitialSync, std::string );
+

gribozavr wrote:
> Make it a static function in the DirectoryWatcher?
You're right - seems like static factory methods are the LLVM way.



Comment at: clang/unittests/DirectoryWatcher/CMakeLists.txt:16
+if(APPLE)
+  check_include_files("CoreServices/CoreServices.h" HAVE_CORESERVICES_H)
+  if(HAVE_CORESERVICES_H)

gribozavr wrote:
> No need to check.  macOS will always have this file.  If it is not there, it 
> is a big issue anyway.
Actually this is wrong but for a different reason - these are link dependencies 
of the implementation. Tests (or any other client) shouldn't care about it.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 2 inline comments as done.
jkorous added a comment.

Thanks for taking a look @gribozavr!

I briefly scanned the rest of your comments and I agree with you (or don't 
really have a strong opinion) in all cases. I'll work on that today.




Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:136
+
+const int PollResult = poll(, 1, TimeoutMs);
+// There are inotify events waiting to be read!

gribozavr wrote:
> What is the role of the timeout and why does it need to be so small?
The whole idea is that we can't block on `read()` if we ever want to stop 
watching the directory, release resources (file descriptors, threads) and 
correctly destruct the DirectoryWatcher instance either
- because of a bug in some other thread in the implementation
- or asynchronous client action (e. g. destructor being called) in main 
application thread

The timeout adds latency in those scenarios.



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:131
+  // the async event handling picks them up. Can make this test flaky.
+  std::this_thread::sleep_for(std::chrono::milliseconds(100)); // 0.1s
+}

gribozavr wrote:
> I'm certain this sleep will be flaky on heavily-loaded CI machines.  If you 
> are going to leave it as a sleep, please make it 1s.  But is there really no 
> better way?
That was exactly my thinking! Honestly, I don't know - I wasn't able to come up 
with any reasonably simple, deterministic approach even on a single platform :(
I eventually picked 0.1s as a tradeoff between slowing the test for everyone 
and getting less false positives.

The problem as I understand it is that we're making changes and monitoring them 
asynchronously with no guarantee from the kernel API (true for FSEvents, not 
100% about inotify) about when (if) we receive notifications.

If you have any idea for robust testing approach I'd be totally happy to use it.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:78
+  /// The DirectoryWatcher that originated this event is no longer valid 
and
+  /// it's behavior is undefined.
+  /// The prime case is kernel signalling to OS-specific implementation of

"its"



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:78
+  /// The DirectoryWatcher that originated this event is no longer valid 
and
+  /// it's behavior is undefined.
+  /// The prime case is kernel signalling to OS-specific implementation of

gribozavr wrote:
> "its"
I'd say "unspecified".  "Undefined behavior" has a specific meaning in C++, and 
I don't believe we have that.

Everywhere in the patch.



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:79
+  /// it's behavior is undefined.
+  /// The prime case is kernel signalling to OS-specific implementation of
+  /// DirectoryWatcher some resource limit being hit.

Please add blank lines between paragraphs (everywhere in the patch).



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:95
+EventKind Kind;
+/// Filename - a relative path to the watched directory or empty string in
+/// case event is related to the directory itself.

Don't repeat field names in comments.



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:95
+EventKind Kind;
+/// Filename - a relative path to the watched directory or empty string in
+/// case event is related to the directory itself.

gribozavr wrote:
> Don't repeat field names in comments.
"a relative path" -- relative to what?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:95
+EventKind Kind;
+/// Filename - a relative path to the watched directory or empty string in
+/// case event is related to the directory itself.

gribozavr wrote:
> gribozavr wrote:
> > Don't repeat field names in comments.
> "a relative path" -- relative to what?
Is it really a path to the directory?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:103
+
+  typedef std::function Events, bool isInitial)>
+  EventReceiver;

"IsInitial"



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:103
+
+  typedef std::function Events, bool isInitial)>
+  EventReceiver;

gribozavr wrote:
> "IsInitial"
Users are not going to use this typedef in their code, so I feel like it is 
obfuscating code -- could you expand it in places where it is used?





Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:123
+   DirectoryWatcher::EventReceiver Receiver,
+   bool WaitInitialSync, std::string );
+

Make it a static function in the DirectoryWatcher?



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:86
+  EventQueue Queue;
+  // flag used for stopping all async acions
+  std::atomic Stop;

Don't write what something is used for (it can change), write what something is.

"If true, all async actions are requested to stop."



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:106
+  FinishedInitScan(false), Receiver(Receiver),
+  InotifyPollingThread([this, InotifyFD]() {
+// We want to be able to read ~30 events at once even in the worst case

I think this is too much code in the initializer list.  Could you move the body 
of the lambda into a method, and call it from this lambda?



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:110
+constexpr size_t EventBufferLength =
+30 * (sizeof(struct inotify_event) + NAME_MAX + 1);
+// http://man7.org/linux/man-pages/man7/inotify.7.html

NAME_MAX is 255, add some for inotify_event, and multiply by 30... we get 
almost 8 Kb on the stack. Should we allocate on the heap instead?





Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:112
+// http://man7.org/linux/man-pages/man7/inotify.7.html
+/* Some systems cannot read integer variables if they are not
+properly aligned. On other systems, incorrect alignment may

Please use "//" comments.



Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:118
+char Buf[EventBufferLength]
+__attribute__((aligned(__alignof__(struct inotify_event;
+

Please use AlignedCharArray from llvm/include/llvm/Support/AlignOf.h



Comment at: 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 200123.
jkorous added a comment.

fix link libraries in cmake


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

https://reviews.llvm.org/D58418

Files:
  clang/cmake/modules/AddClang.cmake
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,359 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+// Intentionally trivial - expecting just a very small numbers of events.
+class TestEventConsumer {
+  std::vector InitialEvents;
+  std::vector NonInitialEvents;
+  sys::Mutex Mtx;
+
+  // Expecting only very small sets - don't bother with anything smart.
+  static bool
+  AreExpectedPresent(const std::vector ,
+ const std::vector ) {
+for (const auto  : expected) {
+  if (std::find(actual.begin(), actual.end(), e) == actual.end())
+return false;
+}
+return true;
+  }
+
+public:
+  void push(llvm::ArrayRef Events, bool isInitial) {
+sys::ScopedLock L(Mtx);
+if (isInitial) {
+  for (const auto  : Events)
+InitialEvents.push_back(E);
+} else {
+  for (const auto  : Events)
+NonInitialEvents.push_back(E);
+}
+  }
+
+  std::vector getInitialEvents() {
+sys::ScopedLock L(Mtx);
+return InitialEvents;
+  }
+  std::vector getNonInitialEvents() {
+sys::ScopedLock L(Mtx);
+return NonInitialEvents;
+  }
+
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInInitial(
+  const std::vector ) {
+sys::ScopedLock L(Mtx);
+return AreExpectedPresent(InitialEvents, expected);
+  }
+
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInNonInitial(
+  const std::vector ) {
+sys::ScopedLock L(Mtx);
+return AreExpectedPresent(NonInitialEvents, expected);
+  }
+};
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+assert(!createUniqueDirectory("dirwatcher", pathBuf));
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+assert(!create_directory(TestWatchedDir, false));
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+Expected ft = openNativeFileForWrite(getPathInWatched(testFile),
+ CD_CreateNew, OF_None);
+if (ft) {
+  closeFile(*ft);
+} else {
+  llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+  llvm::errs() << getPathInWatched(testFile) << "\n";
+  llvm_unreachable("Couldn't create test file.");
+}
+  }
+
+  void deleteFile(const std::string ) {
+std::error_code EC =
+remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+ASSERT_FALSE(EC);
+  }
+};
+
+void WaitForAsync() {
+  // This is just a heuristic - trying to wait for changes to FS to propagate so
+  // the async event handling picks them up. Can make this test flaky.
+  std::this_thread::sleep_for(std::chrono::milliseconds(100)); // 0.1s
+}
+
+} // namespace
+
+TEST(DirectoryWatcherTest, initialScanSync) {
+  

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 199712.
jkorous added a comment.

A major clean-up.

changelog
=

**general**

- specification, documentation, tests
- returning only filenames instead of paths (or empty string when the event is 
related to the watched dir itself)
- removed unsound event deduplication during/right after the initial scan
- simplified how is OS-specific implementation selected

**linux**

- properly terminating threads
- added pthreads to fix shared libs build
- handle IN_DELETE_SELF, IN_IGNORED
- IN_EXCL_UNLINK

**macos**

- simplified synchronization by removing semaphores
- workarounds in FSEvents use

I am not entirely happy about my tests - since we're handling notifications 
from kernel asynchronously it's hard to write a deterministic test. I just use 
some 100 ms sleeps as a workaround. Happy to use something more robust if 
anyone has some ideas. Tests seem fine with msan & tsan on linux and tsan on 
macos but given their scope it doesn't mean that much.


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

https://reviews.llvm.org/D58418

Files:
  clang/cmake/modules/AddClang.cmake
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryScanner.cpp
  clang/lib/DirectoryWatcher/DirectoryScanner.h
  clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
  clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,359 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event ,
+   const DirectoryWatcher::Event ) {
+  return lhs.Filename == rhs.Filename &&
+ static_cast(lhs.Kind) == static_cast(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+// Intentionally trivial - expecting just a very small numbers of events.
+class TestEventConsumer {
+  std::vector InitialEvents;
+  std::vector NonInitialEvents;
+  sys::Mutex Mtx;
+
+  // Expecting only very small sets - don't bother with anything smart.
+  static bool
+  AreExpectedPresent(const std::vector ,
+ const std::vector ) {
+for (const auto  : expected) {
+  if (std::find(actual.begin(), actual.end(), e) == actual.end())
+return false;
+}
+return true;
+  }
+
+public:
+  void push(llvm::ArrayRef Events, bool isInitial) {
+sys::ScopedLock L(Mtx);
+if (isInitial) {
+  for (const auto  : Events)
+InitialEvents.push_back(E);
+} else {
+  for (const auto  : Events)
+NonInitialEvents.push_back(E);
+}
+  }
+
+  std::vector getInitialEvents() {
+sys::ScopedLock L(Mtx);
+return InitialEvents;
+  }
+  std::vector getNonInitialEvents() {
+sys::ScopedLock L(Mtx);
+return NonInitialEvents;
+  }
+
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInInitial(
+  const std::vector ) {
+sys::ScopedLock L(Mtx);
+return AreExpectedPresent(InitialEvents, expected);
+  }
+
+  // Fool-proof way how to compare.
+  bool AreExpectedPresentInNonInitial(
+  const std::vector ) {
+sys::ScopedLock L(Mtx);
+return AreExpectedPresent(NonInitialEvents, expected);
+  }
+};
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+SmallString<128> pathBuf;
+assert(!createUniqueDirectory("dirwatcher", pathBuf));
+TestRootDir = pathBuf.str();
+path::append(pathBuf, "watch");
+TestWatchedDir = pathBuf.str();
+assert(!create_directory(TestWatchedDir, false));
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string ) {
+SmallString<128> pathBuf;
+pathBuf = TestWatchedDir;
+path::append(pathBuf, testFile);
+return pathBuf;
+  }
+
+  void addFile(const std::string ) {
+

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked an inline comment as done.
jkorous added a comment.

In D58418#1431765 , @thakis wrote:

> In D58418#1431399 , @jkorous wrote:
>
> > In D58418#1430630 , @thakis wrote:
> >
> > > In D58418#1430490 , @jkorous 
> > > wrote:
> > >
> > > > In D58418#1430160 , @thakis 
> > > > wrote:
> > > >
> > > > > Why is this needed for index-while-building? My mental model for 
> > > > > index-while-building is that that streams out build index metadata as 
> > > > > part of the regular compile. Why does that require watching 
> > > > > directories?
> > > >
> > > >
> > > > You're right that this isn't necessary for the indexing phase. But we 
> > > > also provide API so clients can consume the index. This functionality 
> > > > is used for getting notifications about index data changes.
> > > >
> > > > You can see it for example here:
> > > >  
> > > > https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111
> > >
> > >
> > > Is that code going to live in clang? This seems more like a tool built on 
> > > top of the compiler rather than something core to the compiler itself 
> > > (like the actual index-while-building feature). Maybe this could be in 
> > > clang-tools-extra?
> >
> >
> > It actually is part of the feature as the serialized format of the index 
> > isn't meant as a stable interface, that's what the API is for. 
> > DirectoryWatcher isn't a tool, it's just part of implementation of the 
> > IndexStore API.
>
>
> Maybe I misunderstand what the client of the IndexStore API is. That's not 
> code that will be in the clang binary, right?


No, it won't.

Currently the client using this API is our indexing service.
https://github.com/apple/sourcekit-lsp
In the future clangd might become another client.

In theory we could have the index producing part in clang and the index 
consuming part (IndexStore) somewhere else (clang-tools-extra?) and use 
functionality that also lives somewhere else (llvm?) but reasons I'd rather not 
do it *NOW* are:

1. We'd have to expose the interface between them (the binary file format) 
which has been just an implementation detail so far without any intention to 
keep it stable.
2. From the general perspective - although I am upstreaming a fully developed 
feature (roughly 10kLOC) it is apparent that I am going to rewrite a 
significant part of the code based on the feedback from reviews. This patch is 
#2 out of approximately 10-15 patches total. Since it's probable that the 
design will change in upcoming reviews I'd prefer to discuss this kind of 
questions after a significant part of the whole design has been through the 
review.
3. For any code that we would move up the tree (to llvm repo) I'd like to have 
a clear use-case other than index-while-building first. Designing generic APIs 
is hard/impossible without known specific use-cases (I think the recommended 
minimum is 3).

The most important word is "now". I am totally happy to discuss this and move 
parts somewhere else if it seems reasonable in the future.

Does that make sense?




Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:27
+/// be invoked even if the directory is empty.
+class DirectoryWatcher {
+public:

gribozavr wrote:
> I feel like since there's nothing Clang-specific about it, it should go into 
> libSupport in LLVM, what do you think?
This has been brought up before. I prefer to leave it here for now since it's 
not used anywhere else. I'd only move it to llvm/Support once we have another 
use-case as that would mean specific requirements for the interface.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

In D58418#1431399 , @jkorous wrote:

> In D58418#1430630 , @thakis wrote:
>
> > In D58418#1430490 , @jkorous wrote:
> >
> > > In D58418#1430160 , @thakis 
> > > wrote:
> > >
> > > > Why is this needed for index-while-building? My mental model for 
> > > > index-while-building is that that streams out build index metadata as 
> > > > part of the regular compile. Why does that require watching directories?
> > >
> > >
> > > You're right that this isn't necessary for the indexing phase. But we 
> > > also provide API so clients can consume the index. This functionality is 
> > > used for getting notifications about index data changes.
> > >
> > > You can see it for example here:
> > >  
> > > https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111
> >
> >
> > Is that code going to live in clang? This seems more like a tool built on 
> > top of the compiler rather than something core to the compiler itself (like 
> > the actual index-while-building feature). Maybe this could be in 
> > clang-tools-extra?
>
>
> It actually is part of the feature as the serialized format of the index 
> isn't meant as a stable interface, that's what the API is for. 
> DirectoryWatcher isn't a tool, it's just part of implementation of the 
> IndexStore API.


Maybe I misunderstand what the client of the IndexStore API is. That's not code 
that will be in the clang binary, right?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D58418#1430630 , @thakis wrote:

> In D58418#1430490 , @jkorous wrote:
>
> > In D58418#1430160 , @thakis wrote:
> >
> > > Why is this needed for index-while-building? My mental model for 
> > > index-while-building is that that streams out build index metadata as 
> > > part of the regular compile. Why does that require watching directories?
> >
> >
> > You're right that this isn't necessary for the indexing phase. But we also 
> > provide API so clients can consume the index. This functionality is used 
> > for getting notifications about index data changes.
> >
> > You can see it for example here:
> >  
> > https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111
>
>
> Is that code going to live in clang? This seems more like a tool built on top 
> of the compiler rather than something core to the compiler itself (like the 
> actual index-while-building feature). Maybe this could be in 
> clang-tools-extra?


It actually is part of the feature as the serialized format of the index isn't 
meant as a stable interface, that's what the API is for. DirectoryWatcher isn't 
a tool, it's just part of implementation of the IndexStore API.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr requested changes to this revision.
gribozavr added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:27
+/// be invoked even if the directory is empty.
+class DirectoryWatcher {
+public:

I feel like since there's nothing Clang-specific about it, it should go into 
libSupport in LLVM, what do you think?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:33
+/// A file gets moved into the directory and replaces an existing file
+/// with the same name will trigger an 'Added' event but no 'Removed' 
event.
+/// If a file gets replaced multiple times within a short time period, it

"A file being moved into ... and replacing ... "



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:43
+Removed,
+/// A file was modified.
+Modified,

"File content was modified." ?  In other words, metadata was not modified?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:46
+/// The watched directory got deleted. No more events will follow.
+DirectoryDeleted,
+  };

`DirectoryRemoved` (for consistency with `Removed`)

Also, how about `TopDirectoryRemoved`?  Otherwise it sounds like some random 
directory was removed.



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:51
+EventKind Kind;
+std::string Filename;
+  };

Please document if it is going to be absolute or relative path, or just the 
file name.




Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:55
+  typedef std::function Events, bool isInitial)>
+  EventReceiver;
+

Users are not going to use this typedef in their code, so I feel like it is 
obfuscating code -- could you expand it in places where it is used?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:61
+  EventReceiver Receiver,
+  bool waitInitialSync,
+  std::string );

`WaitForInitialSync` (everywhere in the patch)



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:62
+  bool waitInitialSync,
+  std::string );
+

Why not return `llvm::ErrorOr>`?

I also don't understand why we need unique_ptr.  If you change `Implementation 
` to `std::unique_ptr Impl;`, `DirectoryWatcher` becomes 
a move-only type, and `create` can return `llvm::ErrorOr`.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:1
+//===- DirectoryWatcher-linux.inc.h - Linux-platform directory listening 
--===//
+//

Please drop the `.h` from the name, for consistency with the rest of LLVM 
(there are no `.inc.h` files, only `.inc`).



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:93
+  30 * (sizeof(struct inotify_event) + NAME_MAX + 1);
+  char buf[EventBufferLength] __attribute__((aligned(8)));
+

Please use `AlignedCharArray` from `llvm/include/llvm/Support/AlignOf.h`.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:93
+  30 * (sizeof(struct inotify_event) + NAME_MAX + 1);
+  char buf[EventBufferLength] __attribute__((aligned(8)));
+

gribozavr wrote:
> Please use `AlignedCharArray` from `llvm/include/llvm/Support/AlignOf.h`.
NAME_MAX is 255, add some for inotify_event, and multiply by 30... we get 
almost 8 Kb on the stack.  Should we allocate on the heap instead?



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:137
+
+bool DirectoryWatcher::Implementation::initialize(StringRef Path,
+  EventReceiver Receiver,

Return `llvm::Error`?



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:162
+  std::thread watchThread(
+  std::bind(runWatcher, pathToWatch, inotifyFD, evtQueue));
+  watchThread.detach();

Use a lambda instead of bind to improve readability?  
https://clang.llvm.org/extra/clang-tidy/checks/modernize-avoid-bind.html



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:175
+std::thread scanThread(runScan);
+scanThread.detach();
+  }

Same comments as for macOS: we need to buffer events from inotify until the 
initial scan completes.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h:1
+//===- DirectoryWatcher-mac.inc.h - Mac-platform directory listening 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

In D58418#1430490 , @jkorous wrote:

> In D58418#1430160 , @thakis wrote:
>
> > Why is this needed for index-while-building? My mental model for 
> > index-while-building is that that streams out build index metadata as part 
> > of the regular compile. Why does that require watching directories?
>
>
> You're right that this isn't necessary for the indexing phase. But we also 
> provide API so clients can consume the index. This functionality is used for 
> getting notifications about index data changes.
>
> You can see it for example here:
>  
> https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111


Is that code going to live in clang? This seems more like a tool built on top 
of the compiler rather than something core to the compiler itself (like the 
actual index-while-building feature). Maybe this could be in clang-tools-extra?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D58418#1430160 , @thakis wrote:

> Why is this needed for index-while-building? My mental model for 
> index-while-building is that that streams out build index metadata as part of 
> the regular compile. Why does that require watching directories?


You're right that this isn't necessary for the indexing phase. But we also 
provide API so clients can consume the index. This functionality is used for 
getting notifications about index data changes.

You can see it for example here:
https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Why is this needed for index-while-building? My mental model for 
index-while-building is that that streams out build index metadata as part of 
the regular compile. Why does that require watching directories?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 190744.
jkorous added a comment.

Remove duplicate comment


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/Config.inc.in
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,327 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef filename, file_status ) {
+SmallString<128> pathBuf;
+pathBuf = TempDir;
+path::append(pathBuf, filename);
+Expected ft =
+openNativeFileForWrite(pathBuf, CD_CreateNew, OF_None);
+ASSERT_TRUE((bool)ft);
+closeFile(*ft);
+
+SmallString<128> newPath;
+

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Sorry for the delay.  I will finish reviewing tomorrow.




Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:9
+/// \file
+/// \brief Utility class for listening for file system changes in a directory.
+//===--===//

This comment only repeats the doc comment for the class, I'd suggest to remove 
it here.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Ping.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added reviewers: kadircet, gribozavr.
jkorous added a comment.

Adding clangd folks in case they want to take a look.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 188808.

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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/Config.inc.in
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,327 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef filename, file_status ) {
+SmallString<128> pathBuf;
+pathBuf = TempDir;
+path::append(pathBuf, filename);
+Expected ft =
+openNativeFileForWrite(pathBuf, CD_CreateNew, OF_None);
+ASSERT_TRUE((bool)ft);
+closeFile(*ft);
+
+SmallString<128> newPath;
+newPath = WatchedDir;
+path::append(newPath, 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 188783.
jkorous added a comment.

- fix the linux implementation
- clang-format


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/Config.inc.in
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,327 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef filename, file_status ) {
+SmallString<128> pathBuf;
+pathBuf = TempDir;
+path::append(pathBuf, filename);
+Expected ft =
+openNativeFileForWrite(pathBuf, CD_CreateNew, OF_None);
+ASSERT_TRUE((bool)ft);
+closeFile(*ft);
+
+

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki added a comment.

Can we please revert this code? One cannot use dynamic_cast on a char *. Did 
you mean reinterpret_cast?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@xbolva00 I prefer to keep it here for now. I suggest we wait with upstreaming 
until an actual need for use in some other project arises. This is what we 
basically did with VFS.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Maybe move it to LLVM? As this watcher may be useful for other projects in the 
future?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 188586.
jkorous added a comment.

- moved checks for CoreServices/inotify to cmake


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/Config.inc.in
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,327 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef filename, file_status ) {
+SmallString<128> pathBuf;
+pathBuf = TempDir;
+path::append(pathBuf, filename);
+Expected ft =
+openNativeFileForWrite(pathBuf, CD_CreateNew, OF_None);
+ASSERT_TRUE((bool)ft);
+closeFile(*ft);
+
+

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 188423.
jkorous marked 4 inline comments as done.
jkorous added a comment.

- Use RetryAfterSignal
- Propagate events from inotify directly
- Remove ModTime
- Add assert for unknown event kinds


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,327 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef filename, file_status ) {
+SmallString<128> pathBuf;
+pathBuf = TempDir;
+path::append(pathBuf, filename);
+Expected ft =
+openNativeFileForWrite(pathBuf, CD_CreateNew, 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 18 inline comments as done.
jkorous added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:116
+
+  DirectoryWatcher::EventKind K = DirectoryWatcher::EventKind::Added;
+  if (ievt->mask & IN_MODIFY) {

jkorous wrote:
> mgorny wrote:
> > I don't think I understand this assumption. Have you any specific event in 
> > mind that is supposed to be treated as added here? If not, maybe it'd be 
> > better to have no default and instead assert that one of the conditions 
> > below matched.
> SGTM. Will rewrite to Optional<> and assert.
Reconsidered and replaced with one-off closure in order not to complicate 
following code with Optional.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

akyrtzi wrote:
> mgorny wrote:
> > akyrtzi wrote:
> > > mgorny wrote:
> > > > akyrtzi wrote:
> > > > > mgorny wrote:
> > > > > > akyrtzi wrote:
> > > > > > > mgorny wrote:
> > > > > > > > jkorous wrote:
> > > > > > > > > mgorny wrote:
> > > > > > > > > > Why? I suppose this deserves a comment.
> > > > > > > > > I'll add this comment:
> > > > > > > > > 
> > > > > > > > > // The file might have been removed just after we received 
> > > > > > > > > the event.
> > > > > > > > Wouldn't that cause removals to be reported twice?
> > > > > > > Not quite sure if it can happen in practice but I'd suggest to 
> > > > > > > accept this as potential occurrence and add it to documentation 
> > > > > > > ("a 'removed' event may be reported twice). I think it is better 
> > > > > > > to handle a definite "fact" (the file doesn't exist) than ignore 
> > > > > > > it and assume the 'removed' event will eventually show up, or try 
> > > > > > > to eliminate the double reporting which would over-complicate the 
> > > > > > > implementation.
> > > > > > > 
> > > > > > > After all, if inotify() is not 100% reliable then there's already 
> > > > > > > the possibility that you'll get a 'removed' event for a file that 
> > > > > > > was not reported as 'added' before.
> > > > > > I see this as a partial workaround for race condition. You 'fix' it 
> > > > > > if removal happens between inotify reporting the event and you 
> > > > > > returning it. You don't if removal happens after you push it to 
> > > > > > events. Therefore, the caller still needs to account for 'added' 
> > > > > > event being obsolete. I don't really see a purpose in trying to 
> > > > > > workaround the problem partially here if the caller still needs to 
> > > > > > account for it. Effectively, you're replacing one normal case and 
> > > > > > one corner case with one normal case and two corner cases.
> > > > > I was mainly pointing out that the client already has to be prepared 
> > > > > for a 'removed' event that does not have an associated 'added' event, 
> > > > > regardless of what this code is doing. Therefore a potential double 
> > > > > 'removed' event doesn't add complexity to the client.
> > > > > 
> > > > > Could you clarify how the code should handle the inability to get the 
> > > > > mod time ? Should it ignore the event ?
> > > > Given the code is supposed to wrap filesystem notification layer, I'd 
> > > > say it should pass the events unmodified and not double-guess what the 
> > > > client expects. The client needs to be prepared for non-atomicity of 
> > > > this anyway.
> > > So it should report an 'added' event but with optional mod-time, 
> > > essentially reporting that something was added that doesn't exist ?
> > > I'd prefer not to do that because it complicates the client without any 
> > > real benefit. It was great that you pointed out this part of the code but 
> > > I'd recommend that if the file is gone we should ignore the 'added' 
> > > event, instead of complicating the API for a corner case.
> > Except that is technically impossible to avoid reporting something that 
> > doesn't exist because it can be removed just after you check for it. So the 
> > client needs to *always* support it, otherwise it's fragile to race 
> > conditions.
> > 
> > This extra check just covers the short period (-> 0) between reporting and 
> > checking. It's needless complexity that doesn't solve the problem. If it 
> > does anything, then it gives you false security that you've solved the 
> > problem when actually the file may still disappear 1 ns after you've 
> > checked that it existed.
> Ok, that's fair, @jkorous I'm fine with removing the mod-time from the 
> DirectoryWatcher API. We can get and report the mod-time at the index-store 
> library layer.
Removed the conditional event kind change and removed ModTime.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154
+errorMsg += llvm::sys::StrError();
+return true;
+  };

dexonsmith wrote:
> jkorous 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> akyrtzi wrote:
> > mgorny wrote:
> > > akyrtzi wrote:
> > > > mgorny wrote:
> > > > > akyrtzi wrote:
> > > > > > mgorny wrote:
> > > > > > > jkorous wrote:
> > > > > > > > mgorny wrote:
> > > > > > > > > Why? I suppose this deserves a comment.
> > > > > > > > I'll add this comment:
> > > > > > > > 
> > > > > > > > // The file might have been removed just after we received the 
> > > > > > > > event.
> > > > > > > Wouldn't that cause removals to be reported twice?
> > > > > > Not quite sure if it can happen in practice but I'd suggest to 
> > > > > > accept this as potential occurrence and add it to documentation ("a 
> > > > > > 'removed' event may be reported twice). I think it is better to 
> > > > > > handle a definite "fact" (the file doesn't exist) than ignore it 
> > > > > > and assume the 'removed' event will eventually show up, or try to 
> > > > > > eliminate the double reporting which would over-complicate the 
> > > > > > implementation.
> > > > > > 
> > > > > > After all, if inotify() is not 100% reliable then there's already 
> > > > > > the possibility that you'll get a 'removed' event for a file that 
> > > > > > was not reported as 'added' before.
> > > > > I see this as a partial workaround for race condition. You 'fix' it 
> > > > > if removal happens between inotify reporting the event and you 
> > > > > returning it. You don't if removal happens after you push it to 
> > > > > events. Therefore, the caller still needs to account for 'added' 
> > > > > event being obsolete. I don't really see a purpose in trying to 
> > > > > workaround the problem partially here if the caller still needs to 
> > > > > account for it. Effectively, you're replacing one normal case and one 
> > > > > corner case with one normal case and two corner cases.
> > > > I was mainly pointing out that the client already has to be prepared 
> > > > for a 'removed' event that does not have an associated 'added' event, 
> > > > regardless of what this code is doing. Therefore a potential double 
> > > > 'removed' event doesn't add complexity to the client.
> > > > 
> > > > Could you clarify how the code should handle the inability to get the 
> > > > mod time ? Should it ignore the event ?
> > > Given the code is supposed to wrap filesystem notification layer, I'd say 
> > > it should pass the events unmodified and not double-guess what the client 
> > > expects. The client needs to be prepared for non-atomicity of this anyway.
> > So it should report an 'added' event but with optional mod-time, 
> > essentially reporting that something was added that doesn't exist ?
> > I'd prefer not to do that because it complicates the client without any 
> > real benefit. It was great that you pointed out this part of the code but 
> > I'd recommend that if the file is gone we should ignore the 'added' event, 
> > instead of complicating the API for a corner case.
> Except that is technically impossible to avoid reporting something that 
> doesn't exist because it can be removed just after you check for it. So the 
> client needs to *always* support it, otherwise it's fragile to race 
> conditions.
> 
> This extra check just covers the short period (-> 0) between reporting and 
> checking. It's needless complexity that doesn't solve the problem. If it does 
> anything, then it gives you false security that you've solved the problem 
> when actually the file may still disappear 1 ns after you've checked that it 
> existed.
Ok, that's fair, @jkorous I'm fine with removing the mod-time from the 
DirectoryWatcher API. We can get and report the mod-time at the index-store 
library layer.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

akyrtzi wrote:
> mgorny wrote:
> > akyrtzi wrote:
> > > mgorny wrote:
> > > > akyrtzi wrote:
> > > > > mgorny wrote:
> > > > > > jkorous wrote:
> > > > > > > mgorny wrote:
> > > > > > > > Why? I suppose this deserves a comment.
> > > > > > > I'll add this comment:
> > > > > > > 
> > > > > > > // The file might have been removed just after we received the 
> > > > > > > event.
> > > > > > Wouldn't that cause removals to be reported twice?
> > > > > Not quite sure if it can happen in practice but I'd suggest to accept 
> > > > > this as potential occurrence and add it to documentation ("a 
> > > > > 'removed' event may be reported twice). I think it is better to 
> > > > > handle a definite "fact" (the file doesn't exist) than ignore it and 
> > > > > assume the 'removed' event will eventually show up, or try to 
> > > > > eliminate the double reporting which would over-complicate the 
> > > > > implementation.
> > > > > 
> > > > > After all, if inotify() is not 100% reliable then there's already the 
> > > > > possibility that you'll get a 'removed' event for a file that was not 
> > > > > reported as 'added' before.
> > > > I see this as a partial workaround for race condition. You 'fix' it if 
> > > > removal happens between inotify reporting the event and you returning 
> > > > it. You don't if removal happens after you push it to events. 
> > > > Therefore, the caller still needs to account for 'added' event being 
> > > > obsolete. I don't really see a purpose in trying to workaround the 
> > > > problem partially here if the caller still needs to account for it. 
> > > > Effectively, you're replacing one normal case and one corner case with 
> > > > one normal case and two corner cases.
> > > I was mainly pointing out that the client already has to be prepared for 
> > > a 'removed' event that does not have an associated 'added' event, 
> > > regardless of what this code is doing. Therefore a potential double 
> > > 'removed' event doesn't add complexity to the client.
> > > 
> > > Could you clarify how the code should handle the inability to get the mod 
> > > time ? Should it ignore the event ?
> > Given the code is supposed to wrap filesystem notification layer, I'd say 
> > it should pass the events unmodified and not double-guess what the client 
> > expects. The client needs to be prepared for non-atomicity of this anyway.
> So it should report an 'added' event but with optional mod-time, essentially 
> reporting that something was added that doesn't exist ?
> I'd prefer not to do that because it complicates the client without any real 
> benefit. It was great that you pointed out this part of the code but I'd 
> recommend that if the file is gone we should ignore the 'added' event, 
> instead of complicating the API for a corner case.
Except that is technically impossible to avoid reporting something that doesn't 
exist because it can be removed just after you check for it. So the client 
needs to *always* support it, otherwise it's fragile to race conditions.

This extra check just covers the short period (-> 0) between reporting and 
checking. It's needless complexity that doesn't solve the problem. If it does 
anything, then it gives you false security that you've solved the problem when 
actually the file may still disappear 1 ns after you've checked that it existed.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> akyrtzi wrote:
> > mgorny wrote:
> > > akyrtzi wrote:
> > > > mgorny wrote:
> > > > > jkorous wrote:
> > > > > > mgorny wrote:
> > > > > > > Why? I suppose this deserves a comment.
> > > > > > I'll add this comment:
> > > > > > 
> > > > > > // The file might have been removed just after we received the 
> > > > > > event.
> > > > > Wouldn't that cause removals to be reported twice?
> > > > Not quite sure if it can happen in practice but I'd suggest to accept 
> > > > this as potential occurrence and add it to documentation ("a 'removed' 
> > > > event may be reported twice). I think it is better to handle a definite 
> > > > "fact" (the file doesn't exist) than ignore it and assume the 'removed' 
> > > > event will eventually show up, or try to eliminate the double reporting 
> > > > which would over-complicate the implementation.
> > > > 
> > > > After all, if inotify() is not 100% reliable then there's already the 
> > > > possibility that you'll get a 'removed' event for a file that was not 
> > > > reported as 'added' before.
> > > I see this as a partial workaround for race condition. You 'fix' it if 
> > > removal happens between inotify reporting the event and you returning it. 
> > > You don't if removal happens after you push it to events. Therefore, the 
> > > caller still needs to account for 'added' event being obsolete. I don't 
> > > really see a purpose in trying to workaround the problem partially here 
> > > if the caller still needs to account for it. Effectively, you're 
> > > replacing one normal case and one corner case with one normal case and 
> > > two corner cases.
> > I was mainly pointing out that the client already has to be prepared for a 
> > 'removed' event that does not have an associated 'added' event, regardless 
> > of what this code is doing. Therefore a potential double 'removed' event 
> > doesn't add complexity to the client.
> > 
> > Could you clarify how the code should handle the inability to get the mod 
> > time ? Should it ignore the event ?
> Given the code is supposed to wrap filesystem notification layer, I'd say it 
> should pass the events unmodified and not double-guess what the client 
> expects. The client needs to be prepared for non-atomicity of this anyway.
So it should report an 'added' event but with optional mod-time, essentially 
reporting that something was added that doesn't exist ?
I'd prefer not to do that because it complicates the client without any real 
benefit. It was great that you pointed out this part of the code but I'd 
recommend that if the file is gone we should ignore the 'added' event, instead 
of complicating the API for a corner case.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

akyrtzi wrote:
> mgorny wrote:
> > akyrtzi wrote:
> > > mgorny wrote:
> > > > jkorous wrote:
> > > > > mgorny wrote:
> > > > > > Why? I suppose this deserves a comment.
> > > > > I'll add this comment:
> > > > > 
> > > > > // The file might have been removed just after we received the event.
> > > > Wouldn't that cause removals to be reported twice?
> > > Not quite sure if it can happen in practice but I'd suggest to accept 
> > > this as potential occurrence and add it to documentation ("a 'removed' 
> > > event may be reported twice). I think it is better to handle a definite 
> > > "fact" (the file doesn't exist) than ignore it and assume the 'removed' 
> > > event will eventually show up, or try to eliminate the double reporting 
> > > which would over-complicate the implementation.
> > > 
> > > After all, if inotify() is not 100% reliable then there's already the 
> > > possibility that you'll get a 'removed' event for a file that was not 
> > > reported as 'added' before.
> > I see this as a partial workaround for race condition. You 'fix' it if 
> > removal happens between inotify reporting the event and you returning it. 
> > You don't if removal happens after you push it to events. Therefore, the 
> > caller still needs to account for 'added' event being obsolete. I don't 
> > really see a purpose in trying to workaround the problem partially here if 
> > the caller still needs to account for it. Effectively, you're replacing one 
> > normal case and one corner case with one normal case and two corner cases.
> I was mainly pointing out that the client already has to be prepared for a 
> 'removed' event that does not have an associated 'added' event, regardless of 
> what this code is doing. Therefore a potential double 'removed' event doesn't 
> add complexity to the client.
> 
> Could you clarify how the code should handle the inability to get the mod 
> time ? Should it ignore the event ?
Given the code is supposed to wrap filesystem notification layer, I'd say it 
should pass the events unmodified and not double-guess what the client expects. 
The client needs to be prepared for non-atomicity of this anyway.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> akyrtzi wrote:
> > mgorny wrote:
> > > jkorous wrote:
> > > > mgorny wrote:
> > > > > Why? I suppose this deserves a comment.
> > > > I'll add this comment:
> > > > 
> > > > // The file might have been removed just after we received the event.
> > > Wouldn't that cause removals to be reported twice?
> > Not quite sure if it can happen in practice but I'd suggest to accept this 
> > as potential occurrence and add it to documentation ("a 'removed' event may 
> > be reported twice). I think it is better to handle a definite "fact" (the 
> > file doesn't exist) than ignore it and assume the 'removed' event will 
> > eventually show up, or try to eliminate the double reporting which would 
> > over-complicate the implementation.
> > 
> > After all, if inotify() is not 100% reliable then there's already the 
> > possibility that you'll get a 'removed' event for a file that was not 
> > reported as 'added' before.
> I see this as a partial workaround for race condition. You 'fix' it if 
> removal happens between inotify reporting the event and you returning it. You 
> don't if removal happens after you push it to events. Therefore, the caller 
> still needs to account for 'added' event being obsolete. I don't really see a 
> purpose in trying to workaround the problem partially here if the caller 
> still needs to account for it. Effectively, you're replacing one normal case 
> and one corner case with one normal case and two corner cases.
I was mainly pointing out that the client already has to be prepared for a 
'removed' event that does not have an associated 'added' event, regardless of 
what this code is doing. Therefore a potential double 'removed' event doesn't 
add complexity to the client.

Could you clarify how the code should handle the inability to get the mod time 
? Should it ignore the event ?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

akyrtzi wrote:
> mgorny wrote:
> > jkorous wrote:
> > > mgorny wrote:
> > > > Why? I suppose this deserves a comment.
> > > I'll add this comment:
> > > 
> > > // The file might have been removed just after we received the event.
> > Wouldn't that cause removals to be reported twice?
> Not quite sure if it can happen in practice but I'd suggest to accept this as 
> potential occurrence and add it to documentation ("a 'removed' event may be 
> reported twice). I think it is better to handle a definite "fact" (the file 
> doesn't exist) than ignore it and assume the 'removed' event will eventually 
> show up, or try to eliminate the double reporting which would over-complicate 
> the implementation.
> 
> After all, if inotify() is not 100% reliable then there's already the 
> possibility that you'll get a 'removed' event for a file that was not 
> reported as 'added' before.
I see this as a partial workaround for race condition. You 'fix' it if removal 
happens between inotify reporting the event and you returning it. You don't if 
removal happens after you push it to events. Therefore, the caller still needs 
to account for 'added' event being obsolete. I don't really see a purpose in 
trying to workaround the problem partially here if the caller still needs to 
account for it. Effectively, you're replacing one normal case and one corner 
case with one normal case and two corner cases.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> jkorous wrote:
> > mgorny wrote:
> > > Why? I suppose this deserves a comment.
> > I'll add this comment:
> > 
> > // The file might have been removed just after we received the event.
> Wouldn't that cause removals to be reported twice?
Not quite sure if it can happen in practice but I'd suggest to accept this as 
potential occurrence and add it to documentation ("a 'removed' event may be 
reported twice). I think it is better to handle a definite "fact" (the file 
doesn't exist) than ignore it and assume the 'removed' event will eventually 
show up, or try to eliminate the double reporting which would over-complicate 
the implementation.

After all, if inotify() is not 100% reliable then there's already the 
possibility that you'll get a 'removed' event for a file that was not reported 
as 'added' before.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:196
+  while (true) {
+if (close(inotifyFD) == -1 && errno == EINTR)
+  continue;

There's some fancy function for this in LLVM. `RetryAfterSignal`, I think.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

jkorous wrote:
> mgorny wrote:
> > Why? I suppose this deserves a comment.
> I'll add this comment:
> 
> // The file might have been removed just after we received the event.
Wouldn't that cause removals to be reported twice?



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher.cpp:95
+
+#if !defined(__has_include)
+#define __has_include(x) 0

jkorous wrote:
> mgorny wrote:
> > Why not use CMake checks? You're already partially using them for 
> > frameworks.
> Do I understand correctly that you suggest doing the check in CMake and 
> propagating the result to preprocessor?
Yes, using `check_include_file` and declaring `HAVE_SOMETHING` in config.h or 
alike.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154
+errorMsg += llvm::sys::StrError();
+return true;
+  };

jkorous wrote:
> mgorny wrote:
> > The return values seem to be confusing. Any reason not to use `true` for 
> > success?
> It seems there's no real consensus * about error/success mapping to bool and 
> since this is just implementation detail (not a public interface) I think 
> it's fine. `llvm::Error` has better semantics but seems a bit heavyweight for 
> this.
> 
> * Just tried this: grep -nr "returns true" clang | grep -i error
Agreed that much of LLVM/Clang uses the C convention where `false`/`0` is 
success.

However, I'm curious whether it might be better to thread `llvm::Error` (and/or 
`llvm::Expected`) through, since you bring it up.  They provide nice assertions 
when the caller forgets to check the result, they're cheap on success, and 
there's no true/false ambiguity.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:94
+   std::shared_ptr evtQueue) {
+#define EVT_BUF_LEN (30 * (sizeof(struct inotify_event) + NAME_MAX + 1))
+  char buf[EVT_BUF_LEN] __attribute__((aligned(8)));

Can this be `constexpr int EventBufferLength` instead of `#define EVT_BUF_LEN`?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 4 inline comments as done.
jkorous added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:116
+
+  DirectoryWatcher::EventKind K = DirectoryWatcher::EventKind::Added;
+  if (ievt->mask & IN_MODIFY) {

mgorny wrote:
> I don't think I understand this assumption. Have you any specific event in 
> mind that is supposed to be treated as added here? If not, maybe it'd be 
> better to have no default and instead assert that one of the conditions below 
> matched.
SGTM. Will rewrite to Optional<> and assert.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> Why? I suppose this deserves a comment.
I'll add this comment:

// The file might have been removed just after we received the event.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154
+errorMsg += llvm::sys::StrError();
+return true;
+  };

mgorny wrote:
> The return values seem to be confusing. Any reason not to use `true` for 
> success?
It seems there's no real consensus * about error/success mapping to bool and 
since this is just implementation detail (not a public interface) I think it's 
fine. `llvm::Error` has better semantics but seems a bit heavyweight for this.

* Just tried this: grep -nr "returns true" clang | grep -i error



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher.cpp:95
+
+#if !defined(__has_include)
+#define __has_include(x) 0

mgorny wrote:
> Why not use CMake checks? You're already partially using them for frameworks.
Do I understand correctly that you suggest doing the check in CMake and 
propagating the result to preprocessor?


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked an inline comment as done.
jkorous added a comment.

Fixed closing of FD.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 187868.
jkorous added a comment.

- handle errors returned by close


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,332 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  file_status stat = stats[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  if (it->Kind != DirectoryWatcher::EventKind::Removed &&
+  it->ModTime != stat.getLastModificationTime())
+hadError =
+err(Twine("filename '" + fname + "' has different mod time"));
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef filename, file_status ) {
+SmallString<128> pathBuf;
+pathBuf 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Thanks for taking a look Michał! I'll try to address the rest of your comments 
asap.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 187860.
jkorous marked 2 inline comments as done.
jkorous added a comment.

- handle EINTR
- assert we are reading whole INotifyEvents


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,332 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  file_status stat = stats[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  if (it->Kind != DirectoryWatcher::EventKind::Removed &&
+  it->ModTime != stat.getLastModificationTime())
+hadError =
+err(Twine("filename '" + fname + "' has different mod time"));
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef 

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 187850.
jkorous added a comment.

- license
- end-of-include-guard comments


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,332 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  file_status stat = stats[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  if (it->Kind != DirectoryWatcher::EventKind::Removed &&
+  it->ModTime != stat.getLastModificationTime())
+hadError =
+err(Twine("filename '" + fname + "' has different mod time"));
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef filename, file_status ) {
+SmallString<128> pathBuf;
+

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

Also, don't forget that `inotify()` is not 100% reliable on Linux, and it can 
miss events under high loads. So technically you should probably include 
periodic directory scans if you are going to rely on this actually reporting 
every file added.




Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:101
+if (numRead == -1) {
+  return; // watcher is stopped.
+}

Looks like you're not handling `EINTR`.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:106
+for (char *p = buf; p < buf + numRead;) {
+  struct inotify_event *ievt = (struct inotify_event *)p;
+  p += sizeof(struct inotify_event) + ievt->len;

Maybe use C++ casts.

Also, I'd add some asserts for whether you've got enough data read.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:116
+
+  DirectoryWatcher::EventKind K = DirectoryWatcher::EventKind::Added;
+  if (ievt->mask & IN_MODIFY) {

I don't think I understand this assumption. Have you any specific event in mind 
that is supposed to be treated as added here? If not, maybe it'd be better to 
have no default and instead assert that one of the conditions below matched.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

Why? I suppose this deserves a comment.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154
+errorMsg += llvm::sys::StrError();
+return true;
+  };

The return values seem to be confusing. Any reason not to use `true` for 
success?



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:193
+return;
+  close(inotifyFD);
+  inotifyFD = -1;

You're ignoring return value.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher.cpp:95
+
+#if !defined(__has_include)
+#define __has_include(x) 0

Why not use CMake checks? You're already partially using them for frameworks.


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

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 187623.
jkorous added a comment.

[DirectoryWatcher] Fix detection of FSEvents for iOS
[DirectoryWatcher][NFC] Doxygen


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

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,333 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  file_status stat = stats[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  if (it->Kind != DirectoryWatcher::EventKind::Removed &&
+  it->ModTime != stat.getLastModificationTime())
+hadError =
+err(Twine("filename '" + fname + "' has different mod time"));
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+std::error_code EC = createUniqueDirectory("dirwatcher", pathBuf);
+ASSERT_FALSE(EC);
+TempDir = pathBuf.str();
+path::append(pathBuf, "watch");
+WatchedDir = pathBuf.str();
+EC = create_directory(WatchedDir);
+ASSERT_FALSE(EC);
+  }
+
+  ~DirectoryWatcherTest() {
+stopWatching();
+remove_directories(TempDir);
+  }
+
+public:
+  StringRef getWatchedDir() const { return WatchedDir; }
+
+  void addFile(StringRef filename, file_status ) {
+

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous created this revision.
jkorous added reviewers: arphaman, dexonsmith, akyrtzi, nathawes, yvvan.
Herald added subscribers: cfe-commits, jdoerfert, jfb, mgorny.
Herald added a project: clang.

This patch contains implementation of DirectoryWatcher from 
github.com/apple/swift-clang

We are starting new push to upstream the index-while-building feature in clang 
and this is one of the dependencies.

Original author is David Farler, other contributors are Argyrios Kyrtzidis, 
Thomas Roughton and Alex Lorenz.

Part of this implementation was included in the review below so I am adding 
@tschuett and @yvvan in case they are interested.
https://reviews.llvm.org/D41407


Repository:
  rC Clang

https://reviews.llvm.org/D58418

Files:
  clang/include/clang/DirectoryWatcher/DirectoryWatcher.h
  clang/lib/CMakeLists.txt
  clang/lib/DirectoryWatcher/CMakeLists.txt
  clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h
  clang/lib/DirectoryWatcher/DirectoryWatcher.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/DirectoryWatcher/CMakeLists.txt
  clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- /dev/null
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -0,0 +1,333 @@
+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace {
+
+class EventCollection {
+  SmallVector Events;
+
+public:
+  EventCollection() = default;
+  explicit EventCollection(ArrayRef events) {
+append(events);
+  }
+
+  void append(ArrayRef events) {
+Events.append(events.begin(), events.end());
+  }
+
+  bool empty() const { return Events.empty(); }
+  size_t size() const { return Events.size(); }
+  void clear() { Events.clear(); }
+
+  bool hasEvents(ArrayRef filenames,
+ ArrayRef kinds,
+ ArrayRef stats) const {
+assert(filenames.size() == kinds.size());
+assert(filenames.size() == stats.size());
+SmallVector evts = Events;
+bool hadError = false;
+for (unsigned i = 0, e = filenames.size(); i < e; ++i) {
+  StringRef fname = filenames[i];
+  DirectoryWatcher::EventKind kind = kinds[i];
+  file_status stat = stats[i];
+  auto it = std::find_if(evts.begin(), evts.end(),
+ [&](const DirectoryWatcher::Event ) -> bool {
+   return path::filename(evt.Filename) == fname;
+ });
+  if (it == evts.end()) {
+hadError = err(Twine("expected filename '" + fname + "' not found"));
+continue;
+  }
+  if (it->Kind != kind) {
+hadError = err(Twine("filename '" + fname + "' has event kind " +
+ std::to_string((int)it->Kind) + ", expected ") +
+   std::to_string((int)kind));
+  }
+  if (it->Kind != DirectoryWatcher::EventKind::Removed &&
+  it->ModTime != stat.getLastModificationTime())
+hadError =
+err(Twine("filename '" + fname + "' has different mod time"));
+  evts.erase(it);
+}
+for (const auto  : evts) {
+  hadError = err(Twine("unexpected filename '" +
+   path::filename(evt.Filename) + "' found"));
+}
+return !hadError;
+  }
+
+  bool hasAdded(ArrayRef filenames,
+ArrayRef stats) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Added};
+return hasEvents(filenames, kinds, stats);
+  }
+
+  bool hasRemoved(ArrayRef filenames) const {
+std::vector kinds{
+filenames.size(), DirectoryWatcher::EventKind::Removed};
+std::vector stats{filenames.size(), file_status{}};
+return hasEvents(filenames, kinds, stats);
+  }
+
+private:
+  bool err(Twine msg) const {
+SmallString<128> buf;
+llvm::errs() << msg.toStringRef(buf) << '\n';
+return true;
+  }
+};
+
+struct EventOccurrence {
+  std::vector Events;
+  bool IsInitial;
+};
+
+class DirectoryWatcherTest
+: public std::enable_shared_from_this {
+  std::string WatchedDir;
+  std::string TempDir;
+  std::unique_ptr DirWatcher;
+
+  std::condition_variable Condition;
+  std::mutex Mutex;
+  std::deque EvtOccurs;
+
+public:
+  void init() {
+SmallString<128> pathBuf;
+