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