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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd marked 3 inline comments as done.
compnerd added inline comments.


================
Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:92
+    Length = GetFinalPathNameByHandleW(DirectoryHandle, Buffer.data(),
+                                       Buffer.capacity(), 0);
+    Buffer.resize(Length);
----------------
amccarth wrote:
> Using `Buffer.data()` when you've only reserved space is undefined behavior.  You should used `resize` instead of `reserve` and then pass the `size` rather than the `capacity`.
> 
> Be aware that, while unlikely, this could still fail.  The directory could have been removed or renamed between calls or the caller could have passed a bad handle to begin with.
Didn't know that about the `reserve`!

Right, that is why most of this is ignoring failures.  The watch will fail and terminate everything.


================
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:
> compnerd wrote:
> > 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.
> But it's still an arbitrarily-sized buffer in the middle of a class definition.  If you change your mind about how big to make it, it changes the definition of the class.  The buffer is going to be accessed using pointer arithmetic, which is generally dangerous.  Moving the buffer out of the class avoids both of those problems.
> 
> The alignas here is _not_ pedantic, it's essential.  Without it, you could easily have an alignment problem.  But if you used a vector, you'd know that it would always be suitably aligned.
I think that the class size point is what is more convincing to me for switching to a `vector`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88666