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