labath accepted this revision. labath added inline comments.
================ Comment at: lldb/tools/driver/Driver.cpp:492-496 #ifdef _WIN32 - _close(fds[WRITE]); - fds[WRITE] = -1; + _close(fd); #else - close(fds[WRITE]); - fds[WRITE] = -1; + close(fd); #endif ---------------- amccarth wrote: > labath wrote: > > Do you think it would be possible to use > > `llvm::Process::SafelyCloseFileDescriptor` here? It looks like that still > > uses close (without the `_`) on windows, which we are trying hard to avoid > > here, but I'm not sure why. I know close is technically deprecated on > > windows, but AFAIK the only thing deprecated about it is the name, and > > otherwise it works correctly. > > > > (If that works, I'd even consider removing this function entirely, as the > > only thing it does extra is clear the fd, but that does not seem necessary > > since we're now calling it immediately before we return (and the fds go out > > of scope anyway)). > I'm happy to use `llvm::sys::Process::SafelyCloseFileDescriptor` and delete > my little wrapper. I agree that resetting the df to -1 right before it goes > out of scope isn't useful. I'll update the patch momentarily. > > If we're not that worried about the deprecated names on Windows, should I > also just use `fdopen` and forget about wrapping that as well? > > Either way, I'm going to keep the OpenPipe wrapper since the Windows version > takes additional parameters. I think using the non-underscore fdopen is fine, but I am not a windows guy. (The nice thing about SafelyCloseFileDescriptor is that it already provides an abstraction and so we can add the underscore there centrally, should it ever become necessary). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60152/new/ https://reviews.llvm.org/D60152 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
