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

Fix issue 9583 #1159

Merged
merged 3 commits into from Feb 26, 2013
Merged

Fix issue 9583 #1159

merged 3 commits into from Feb 26, 2013

Conversation

quickfur
Copy link
Member

std.getopt.getopt should consume options terminator "--" as claimed by docs

if (endOfOptions.length && a == endOfOptions)
{
// Consume the "--"
args = args[0 .. i + 1] ~ args[i + 2 .. $];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args = args.remove(i + 1);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yah

@ghost
Copy link

ghost commented Feb 24, 2013

@alexrp
Copy link
Member

alexrp commented Feb 25, 2013

LGTM other than @AndrejMitrovic's comment.

@alexrp
Copy link
Member

alexrp commented Feb 25, 2013

cc @andralex

@quickfur
Copy link
Member Author

Done.

@ghost
Copy link

ghost commented Feb 25, 2013

Hmm.. The loop is foreach (i, a ; args[1 .. $]). Is it safe to modify args length within the loop?
Edit: Or maybe that break exits the loop so it doesn't matter.

@quickfur
Copy link
Member Author

Generally, I wouldn't do it, but in this case, we're breaking immediately, so I think it should be OK. (In the case of arrays, though, I think foreach actually iterates over a slice, so it should be safe -- modifying args will reallocate anyway so it won't stomp on what the loop is doing.)

@ghost
Copy link

ghost commented Feb 25, 2013

modifying args will reallocate anyway

remove doesn't actually reallocate, it modifies the contents of the slice.

@quickfur
Copy link
Member Author

Oops, you're right. Well, but I think in this case it should be OK, because we're breaking immediately. Unless you think it's better to rewrite the loop to prevent possible future breakage?

@ghost
Copy link

ghost commented Feb 25, 2013

It should be ok since we're breaking. Anyway this pull is LGTM.

@quickfur
Copy link
Member Author

Rebased.

andralex added a commit that referenced this pull request Feb 26, 2013
@andralex andralex merged commit d6af628 into dlang:master Feb 26, 2013
@quickfur quickfur deleted the getopt_optend branch July 16, 2014 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants