[Lldb-commits] [PATCH] D60152: Fix and simplify PrepareCommandsForSourcing

[Pavel Labath via Phabricator via lldb-commits]

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