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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
