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

[Zachary Turner via Phabricator via lldb-commits]

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