[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

I'm sorry.  I haven't had to time to review the entire change yet, but I thought I'd share some early feedback now and take more of a look on Monday.

The high level issues on my mind:

I'm wondering whether this has been overcomplicated with the overlapped IO.  If the monitoring thread used `FindFirstChangeNotificationW` to get a waitable handle and then used, then you'd be able to call `ReadDirectoryChangesW` synchronously.  In order to allow the parent thread signal to quit, they'd just need an Event and the monitor thread would use `WaitForMultipleObjects` to wait for either handle to become signaled.  Maybe I'm overlooking something, but it might be worth a few minutes of consideration.

We'll also have to think about how to test this.

The lower level issues that I've spotted are inlined.



================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:33
 class DirectoryWatcherWindows : public clang::DirectoryWatcher {
+  OVERLAPPED ovlIO;
+
----------------
The `ovlIO` name isn't consistent with LLVM style.


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:36
+  alignas(DWORD)
+  CHAR Buffer[4 * (sizeof(FILE_NOTIFY_INFORMATION) + MAX_PATH * sizeof(WCHAR))];
+
----------------
If it were me, I'd probably make this a `std::vector`.

* If an off-by-one bug causes an overrun of one WCHAR, you could trash a crucial member variable.  On the heap, the damage is less likely to be catastrophic.
* You wouldn't need `alignas`.
* I don't think these are created in a tight loop, so the overhead doesn't concern me.

Also, I'd probably go with a slightly more descriptive name, like `Notifications` rather than `Buffer`.


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:82
+    DirectoryWatcherCallback Receiver)
+    : Callback(Receiver) {
+  // Pre-compute the real location as we will be handing over the directory
----------------
There's a lot going on in this constructor.  Is this how the other implementations are arranged?

Would it make sense to just initialize the object, and save most of the actual work to a `Watch` method?


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:87
+    DWORD dwLength = GetFinalPathNameByHandleW(hDirectory, NULL, 0, 0);
+    std::unique_ptr<WCHAR[]> buffer{new WCHAR[dwLength + 1]};
+    (void)GetFinalPathNameByHandleW(hDirectory, buffer.get(), dwLength + 1, 0);
----------------
compnerd wrote:
> aaron.ballman wrote:
> > Is a smart pointer required here or could you use `std::vector<WCHAR>` and reserve the space that way?
> Sure, I can convert this to a `std::vector<WCHAR>` instead.
* I guess it's fine to use the array form of `std::unique_ptr` (but then you should `#include <memory>`).  If it were me, I'd probably just use a `std::wstring` or `std::vector<WCHAR>`.

* `dwLength` already includes the size of the null terminator.  Your first `GetFinalPathNameByHandleW` function "fails" because the buffer is too small.  The does says that, if it fails because the buffer is too small, then the return value is the required size _including_ the null terminator.  (In the success case, it's the size w/o the terminator.)

* I know this is the Windows-specific implementation, but it might be best to just the Support api ` realPathFromHandle`, which does this and has tests.


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:88
+    std::unique_ptr<WCHAR[]> buffer{new WCHAR[dwLength + 1]};
+    (void)GetFinalPathNameByHandleW(hDirectory, buffer.get(), dwLength + 1, 0);
+    sys::windows::UTF16ToUTF8(buffer.get(), dwLength + 1, Path);
----------------
I don't think you want to ignore the return value, since it'll tell you exactly how many characters you actually got back (or whether there was an error).  Again, I recommend using `realPathFromHandle` from Support.


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:94
+  ovlIO.hEvent =
+      CreateEvent(NULL, /*bManualReset=*/TRUE, /*bInitialState=*/FALSE, NULL);
+  assert(ovlIO.hEvent);
----------------
No real difference here, but, for consistency, please make this `CreateEventW` with the explicit -W suffix.


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:189
+  HandlerThread.join();
+  WatcherThread.join();
+}
----------------
I think this leaks the `ovlIO.hEvent`.  After you've joined both threads, make sure to call `::CloseHandle()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88666