Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve portability of getopt reset #1545

Closed
wants to merge 2 commits into from

Conversation

bonzini
Copy link

@bonzini bonzini commented Mar 24, 2019

getopt reset is done sometimes with "optind = 0", sometimes with "optind = 1". Unfortunately, neither is portable. For example, if the bundled getopt see "optind = 1" on the very first call it will implicitly add a dash in front of the first argument; and while some glibc versions even had reports of crashes if reset with "optind = 1", Apple's getopt did the same for "optind = 0".

Since we know that there will always be two getopt calls only, of which the first will be in ReadFlags, place the ugly code there so that it is not replicated all over ninja.cc.

Avoid duplication across multiple files by defining a symbol
USE_BUNDLED_GETOPT in configure.py, and clean up the resulting #include
directives.
getopt reset is done sometimes with "optind = 0", sometimes with
"optind = 1".  Unfortunately, neither is portable.  For example,
if the bundled getopt see "optind = 1" on the very first call it will
implicitly add a dash in front of the first argument; and while some
glibc versions even crashed if reset with "optind = 1", Apple's
getopt did the same for "optind = 0".

Since we know that there will always be two getopt calls only, of
which the first will be in ReadFlags, place the ugly code there
so that it is not replicated all over ninja.cc.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
@jhasse
Copy link
Collaborator

jhasse commented Mar 24, 2019

Why not use the bundled getopt all the time?

@bonzini
Copy link
Author

bonzini commented Mar 25, 2019

That would be a questioni for you. :-) There could be prototype compatibility issues I guess, similar to AIX.

@jhasse
Copy link
Collaborator

jhasse commented Mar 25, 2019

That would be a questioni for you. :-)

I didn't write any of this, so I have no idea.

There could be prototype compatibility issues I guess

You mean that the definition of getopt might conflict with the system provided one?

@bonzini
Copy link
Author

bonzini commented Mar 25, 2019

Yes, for example const differences could be a problem.

@evmar
Copy link
Collaborator

evmar commented Mar 26, 2019

I don't quite remember, but I think I first wrote the code assuming that getopt was always available, then once we were on a system that didn't have it we added the builtin one. I agree that maybe always using the builtin one is a good idea.

@jhasse
Copy link
Collaborator

jhasse commented Jan 18, 2020

Always using the builtin one is the way to go IMHO. Eventually even move on to a different solution with a better API (to make stuff like #1546 easier).

@jhasse jhasse closed this Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants