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