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