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