labath added inline comments. ================ Comment at: include/lldb/Host/HostGetOpt.h:11 @@ -10,3 +10,1 @@ -#ifndef _MSC_VER - ---------------- 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.
================ Comment at: include/lldb/Host/common/GetOptInc.h:7 @@ +6,3 @@ +#define REPLACE_GETOPT +#define REPLACE_GETOPT_LONG +#endif ---------------- This macro will be defined twice for _MSC_VER. You only need one of these two definitions. ================ Comment at: include/lldb/Host/common/GetOptInc.h:10 @@ +9,3 @@ +#if defined(_MSC_VER) || defined(__NetBSD__) +#define REPLACE_GETOPT_LONG +#endif ---------------- second definition. ================ Comment at: source/Host/CMakeLists.txt:12 @@ -11,2 +11,3 @@ common/FileSystem.cpp + common/GetOptInc.cpp common/Host.cpp ---------------- 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. 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
