[Lldb-commits] [PATCH] D18287: Don't try to redefine the signal() function on Windows

[Cameron via lldb-commits]

cameron314 added inline comments.

================
Comment at: tools/driver/Driver.cpp:1314
@@ -1313,1 +1313,3 @@
+    signal(SIGINT, sigint_handler);
+#ifndef _MSC_VER
     signal (SIGPIPE, SIG_IGN);
----------------
zturner wrote:
> I think `_MSC_VER` is the right check, because the builtin `signal` implementation is something that is part of Visual Studio.  If you were using Cygwin I imagine some of those constants might be present (although tbh, I don't ever use Cygwin and I don't know if anyone else does either, so someone would need to confirm).
> 
> `_MSC_VER` is also defined for clang-cl, so this check would catch MSVC and clang-cl
I was thinking more of mingw-w64, which tries very hard to have a compatible set of headers as those defined in the Windows SDK (and does not define `_MSC_VER`). But many, many other places in LLDB already check `_MSC_VER` anyway.

================
Comment at: tools/driver/Platform.cpp:93-97
@@ -92,7 @@
-    {
-    case ( SIGINT ):
-        {
-            _ctrlHandler = sigFunc;
-            SetConsoleCtrlHandler( CtrlHandler, TRUE );
-        }
-        break;
----------------
zturner wrote:
> Yes, because now `MIDriverMain.cpp` should end up calling the builtin version of `signal()`, which also calls `SetConsoleCtrlHandler`.  As far as I can tell there's no actual difference (technically, the builtin version is a strict superset of this version, so if anything it should be better)
Great!

================
Comment at: tools/driver/Platform.h:40
@@ -39,14 +39,3 @@
 
-
-    // signal handler function pointer type
-    typedef void(*sighandler_t)(int);
-
-    // signal.h
-    #define SIGINT 2
-    // default handler
-    #define SIG_DFL ( (sighandler_t) -1 )
-    // ignored
-    #define SIG_IGN ( (sighandler_t) -2 )
-
     // signal.h
     #define SIGPIPE  13
----------------
amccarth wrote:
> Now that we are including <signal.h>, won't the following defines create duplicate definition warnings for the non-Windows platforms?
It's only included on Windows, though -- unless some other TU includes both this header and `signal.h`?


http://reviews.llvm.org/D18287