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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd marked 14 inline comments as done.
compnerd added a comment.

There already is testing coverage for this - I just missed the CMake changes.



================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:36
+  alignas(DWORD)
+  CHAR Buffer[4 * (sizeof(FILE_NOTIFY_INFORMATION) + MAX_PATH * sizeof(WCHAR))];
+
----------------
amccarth wrote:
> 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`.
The `alignas` is because the documentation states that the buffer should be DWORD aligned.  It is more for pedantic reasons rather than anything else.  I think that making it a catastrophic failure is a good thing though - it would catch the error :)  You are correct about the allocation - it is once per watch.  I'll rename it at least.


================
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
----------------
amccarth wrote:
> 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?
Largely the same.  However, the majority of the "work" is actually the thread proc for the two threads.


================
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);
----------------
amccarth wrote:
> 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.
I didn't know about `realPathFromHandle` - I prefer that actually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88666