zturner 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 ---------------- labath wrote: > 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). I dug into this a little bit to find out what the actual difference is, and there isn't a difference at all. `close` and `_close`, `fdopen` and `_fdopen`, and all other pairs of underscore and non underscore names both resolve to the exact same symbol in the object file (it links in an object file that has a symbol for `close` that aliases it to `_close`. So there is literally no difference. Repository: rL LLVM 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
