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

[Saleem Abdulrasool via Phabricator via cfe-commits]

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


================
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:
> compnerd wrote:
> > 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.
> Let me put it another way.  Constructors cannot return errors and LLVM does use exceptions, so things that can fail generally shouldn't be in the constructor.  This code is accessing the file system, creating and event, and spawning two threads.  Any of those things can fail, but you've got no way to let the caller know whether something went wrong.
> 
> If the lambdas were really short, then it would be easy to see that they're thread procs.  But they're not, so they're hard to find and understand.  If they were private member functions with descriptive names, the code would be easier to understand.
Sure, moving the thread procs into a member function is reasonable.   Making that request would've been more clear :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88666