Skip to content

Removed comma support for -i (-i=a,b becomes -i=a -i=b)#7857

Merged
dlang-bot merged 1 commit intodlang:masterfrom
marler8997:dashINoCommas
Feb 13, 2018
Merged

Removed comma support for -i (-i=a,b becomes -i=a -i=b)#7857
dlang-bot merged 1 commit intodlang:masterfrom
marler8997:dashINoCommas

Conversation

@marler8997
Copy link
Contributor

@marler8997 marler8997 commented Feb 9, 2018

@WalterBright has suggested that supporting multiple arguments for a single option using the comma character is "a lot of effort". He also mentions other command-line options already support multiple arguments because they can be given multiple times. This PR removes comma support for -i so the following

-i=foo,-foo.bar,baz

must now be specified via:

-i=foo i=-foo.bar -i=baz

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@marler8997 marler8997 force-pushed the dashINoCommas branch 2 times, most recently from c06cf7f to 0e7d05c Compare February 9, 2018 16:43
@marler8997 marler8997 mentioned this pull request Feb 9, 2018
@wilzbach
Copy link
Contributor

wilzbach commented Feb 9, 2018

(If we go with this, changelog and documentation should be updated too.)
Luckily everything is now in this repo.

@wilzbach wilzbach added this to the 2.079.0 milestone Feb 9, 2018
@marler8997
Copy link
Contributor Author

I've modified the changelog, and we can delay integrating the documentation PR until this PR is settled.

Copy link
Contributor

@JinShil JinShil left a comment

Choose a reason for hiding this comment

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

Comma or no comma is fine with me.

@thewilsonator
Copy link
Contributor

@WalterBright has suggested that supporting multiple arguments for a single option using the comma character is "a lot of effort".

Really? It shouldn't be hard to do what we have in LDC, namely a generic comma separated command line option parser.

@timotheecour
Copy link
Contributor

timotheecour commented Feb 10, 2018

totally agree and strongly support comma separated list of arguments for this flag and others. I planned to add one for Comma separated linker flags to ease adapting these from, say, clang or gcc -Wl,-lfoo1,-lfoo2

ldc, clang and others do it for good reason. Some push back desirable here. Original cmdline design had a number of issues (eg flags without = separator eg -ofbar is just bad). Allowing comma separated flags improve things a bit. If parsing for all comma separated flags was reusing some common code, maybe it would ease some of walters concerns.

@marler8997
Copy link
Contributor Author

If parsing for all comma separated flags was reusing some common code, maybe it would ease some of walters concerns.

I'll create a PR to see how complex this would be and we can see what Walter thinks.

@marler8997
Copy link
Contributor Author

Ok, PR is here: https://github.com/dlang/dmd/pull/7863/files

The implementation creates a struct called OptionRange that takes care of splitting an option's arguments into a range of null-terminated strings, so then when you parse the option all you have to do is pass the argument to this OptionRange and iterate over all the values.

@dlang-bot dlang-bot merged commit 11fc856 into dlang:master Feb 13, 2018
dmd -i=-std,-core,-etc,-object
---

$(CONSOLE dmd -i=-std,-core,-etc,-object)
Copy link
Member

Choose a reason for hiding this comment

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

I thought this syntax was removed with this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, unless #7863 gets merged before we release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shoot, missed that one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ZombineDev do you want to make a PR for that one or shall I? I think at this point it's gotta be merged to the stable branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants