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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I wonder if we should unit test this functionality by having some tests that create and remove files that are watched. I'm not 100% convinced that is a great idea, but not having test coverage for the change is also not really a great idea either. Thoughts welcome.



================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:22
+
+#include <Windows.h>
 
----------------
You should include `llvm/Support/Windows/WindowsSupport.h` not `Windows.h` directly.


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:71
 public:
-  ~DirectoryWatcherWindows() override { }
-  void InitialScan() { }
-  void EventReceivingLoop() { }
-  void StopWork() { }
+  DirectoryWatcherWindows(HANDLE hDirectory, bool WaitForInitialSync,
+                          DirectoryWatcherCallback Receiver);
----------------
`hDirectory` -> `Directory`


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:80
+DirectoryWatcherWindows::DirectoryWatcherWindows(
+    HANDLE hDirectory, bool WaitForInitialSync,
+    DirectoryWatcherCallback Receiver)
----------------
`hDirectory` -> `Directory`


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:86
+  {
+    DWORD dwLength = GetFinalPathNameByHandleW(hDirectory, NULL, 0, 0);
+    std::unique_ptr<WCHAR[]> buffer{new WCHAR[dwLength + 1]};
----------------
You should strip the Hungarian notation prefixes and ensure all the identifiers meet our usual naming rules, I'll stop bringing them up.


================
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);
----------------
Is a smart pointer required here or could you use `std::vector<WCHAR>` and reserve the space that way?


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:101
+      // subdirectories, might as well as ...
+      const BOOL bWatchSubtree = TRUE;
+      const DWORD dwNotifyFilter = FILE_NOTIFY_CHANGE_FILE_NAME
----------------
You can drop the top-level `const` on value types.


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:170
+    InitialScan();
+  HandlerThread = std::thread([this, WaitForInitialSync]() {
+    // If we did not wait for the initial sync, then we should perform the
----------------
A newline above this line would be helpful for visual distinction.


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:223
+
+  // FIXME(compnerd) should we assert that the path is a directory?
+  SmallVector<wchar_t, MAX_PATH> WidePath;
----------------
I think we should assert that -- calling this on a file isn't likely to behave in a good way.


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:229
+
+  const DWORD dwDesiredAccess = FILE_LIST_DIRECTORY;
+  const DWORD dwShareMode =
----------------
More top-level `const`s


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88666