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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
