[Lldb-commits] [PATCH] D12582: NetBSD doesn't ship with getopt_long_only(3)

[Kamil Rytarowski via lldb-commits]

krytarowski added inline comments.

================
Comment at: include/lldb/Host/HostGetOpt.h:11
@@ -10,3 +10,1 @@
 
-#ifndef _MSC_VER
-
----------------
labath wrote:
> krytarowski wrote:
> > labath wrote:
> > > How about just putting here
> > > ```
> > > #if !defined(_MSC_VER) && !defined(__NetBSD__)
> > > ```
> > I dislike having two headers for the same purpose, can I just obsolete GetOptInc.h and put its content here?
> I kinda like the idea where one header just select the correct implementation to use, and then the other headers contain the actual implementations. So it's not exactly the _same_ purpose for me. But I don't feel too strongly about that, so if you think it will make things cleaner, i guess you can go ahead.
I will do it your way.

================
Comment at: include/lldb/Host/common/GetOptInc.h:10
@@ +9,3 @@
+#if defined(_MSC_VER) || defined(__NetBSD__)
+#define REPLACE_GETOPT_LONG
+#endif
----------------
labath wrote:
> second definition.
Good catch, it must be REPLACE_GETOPT_LONG_ONLY here.

================
Comment at: source/Host/CMakeLists.txt:12
@@ -11,2 +11,3 @@
   common/FileSystem.cpp
+  common/GetOptInc.cpp
   common/Host.cpp
----------------
labath wrote:
> krytarowski wrote:
> > labath wrote:
> > > This will compile the file for all targets, which causes errors e.g. on linux. Either make the inclusion of this file conditional in cmake, or put the entire cpp file contents under appropriate ifdefs (windows or netbsd).
> > I will put the entire file under #ifdefs, I will reuse the defines REPLACE_GETOPT from .h.
> > 
> > For platforms with all needed getopt(3) functions. I will add a dummy local variable, like static int getopt_dummy = 0; This will be needed to feed ld(1) on some toolchains.
> > I will put the entire file under #ifdefs, I will reuse the defines REPLACE_GETOPT from .h.
> Sounds good.
> 
> > For platforms with all needed getopt(3) functions. I will add a dummy local variable, like static int getopt_dummy = 0; This will be needed to feed ld(1) on some toolchains.
> Why exactly is this necessary? Is there a linker that will not accept an empty .o file? We have a number of files #ifdefed completely and it seems to work fine (e.g. source/Plugins/Process/Linux/NativeRegisterContext*). So, unless you know of a particular linker that needs this thing I would leave it out for now.
SunOS used to be one. But unless it's not supported I won't add this linker hack.


Repository:
  rL LLVM

http://reviews.llvm.org/D12582