This is an archive of the discontinued LLVM Phabricator instance.

Move GetOptInc to the common namespace
ClosedPublic

Authored by krytarowski on Sep 2 2015, 6:11 PM.

Details

Summary

GetOptInc provides getopt(), getopt_long() and getopt_long_only().

Windows (for defined(_MSC_VER)) doesn't ship with all of the getopt(3) family members and needs all of them. NetBSD requires only getopt_long_only(3).

While there fix the code for clang diagnostics.

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski updated this revision to Diff 33896.Sep 2 2015, 6:11 PM
krytarowski retitled this revision from to NetBSD doesn't ship with getopt_long_only(3).
krytarowski updated this object.
krytarowski added a reviewer: joerg.
krytarowski set the repository for this revision to rL LLVM.
krytarowski added a subscriber: lldb-commits.

Since we already have an implementation of getopt for windows, couldn't you use that one (or improve/replace it if it is missing some features) for netbsd also? I don't see why it would be necessary to have two independent reimplementations of getopt in the tree...

I will reuse the Windows version - it comes from some BSD and it's modified.

At the moment I don't have a platform to test build it. Please verify whether everything is fine.

krytarowski updated this revision to Diff 33922.Sep 3 2015, 3:34 AM

Reuse the Windows' GetOptInc.

krytarowski added a comment.EditedSep 3 2015, 3:57 AM

In case that new commit message was lost, it's here:

commit 4193d823f0fdda0799c632c05836c2ed8f26186c
Author: Kamil Rytarowski <>
Date: Thu Sep 3 12:10:33 2015 +0200

Move GetOptInc to the common namespace

GetOptInc provides getopt(), getopt_long() and getopt_long_only().

Windows (for defined(_MSC_VER)) doesn't ship with all of them.
NetBSD just needs getopt_long_only().

While there fix the code for clang diagnostics.
labath added a comment.Sep 3 2015, 4:40 AM

Thanks for merging the two implementations. However, the way you have implemented it enables the getopt replacement for all platforms. We need to have it on only for platforms that don't have a native one.

include/lldb/Host/HostGetOpt.h
11 ↗(On Diff #33922)

How about just putting here

#if !defined(_MSC_VER) && !defined(__NetBSD__)
source/Host/CMakeLists.txt
12 ↗(On Diff #33922)

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've inlined replies to your comments.

include/lldb/Host/HostGetOpt.h
11 ↗(On Diff #33922)

I dislike having two headers for the same purpose, can I just obsolete GetOptInc.h and put its content here?

source/Host/CMakeLists.txt
12 ↗(On Diff #33922)

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.

labath added inline comments.Sep 3 2015, 5:56 AM
include/lldb/Host/HostGetOpt.h
11 ↗(On Diff #33922)

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.

include/lldb/Host/common/GetOptInc.h
7 ↗(On Diff #33922)

This macro will be defined twice for _MSC_VER. You only need one of these two definitions.

10 ↗(On Diff #33922)

second definition.

source/Host/CMakeLists.txt
12 ↗(On Diff #33922)

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.

krytarowski added inline comments.Sep 3 2015, 6:11 AM
include/lldb/Host/HostGetOpt.h
11 ↗(On Diff #33922)

I will do it your way.

include/lldb/Host/common/GetOptInc.h
10 ↗(On Diff #33922)

Good catch, it must be REPLACE_GETOPT_LONG_ONLY here.

source/Host/CMakeLists.txt
12 ↗(On Diff #33922)

SunOS used to be one. But unless it's not supported I won't add this linker hack.

krytarowski updated this revision to Diff 33941.Sep 3 2015, 7:11 AM

Apply the latest comments to this patch.

labath added a comment.Sep 3 2015, 7:25 AM

Looks good after fixing inverting one condition, but I would like to wait for an ok from zachary also. Linux builds fine after this patch.

source/Host/common/GetOptInc.cpp
3 ↗(On Diff #33941)

You have the logic reversed here. This should be #if defined(FOO) || defined(BAR) || defined(BAZ)

krytarowski updated this revision to Diff 33944.Sep 3 2015, 8:05 AM

Inverse wrong logic

krytarowski retitled this revision from NetBSD doesn't ship with getopt_long_only(3) to Move GetOptInc to the common namespace.Sep 3 2015, 11:43 AM
krytarowski updated this object.
krytarowski updated this object.

Thank you.

When the review will be done, please commit it.

This revision was automatically updated to reflect the committed changes.
lldb/trunk/source/Host/common/GetOptInc.cpp