Skip to content

Work On Issue 18172 - add scope to pointers in getopt to allow @safe #6281

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Work On Issue 18172 - add scope to pointers in getopt to allow @safe #6281

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions std/getopt.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ parses foo but leaves "--bar" in $(D args). The double-dash itself is
removed from the argument array unless the $(D std.getopt.config.keepEndOfOptions)
directive is given.
*/
GetoptResult getopt(T...)(ref string[] args, T opts)
GetoptResult getopt(T...)(ref string[] args, scope T opts)
{
import std.exception : enforce;
enforce(args.length,
Expand Down Expand Up @@ -459,6 +459,19 @@ GetoptResult getopt(T...)(ref string[] args, T opts)
}
}

version(DIP1000)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how we want to handle DIP1000 regressions, but we need a way to ensure things stay @safe

Copy link
Member

Choose a reason for hiding this comment

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

By #6278 and thus gradually enabling -dip1000?

Copy link
Member Author

Choose a reason for hiding this comment

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

But this unittest needs to be annotated with @safe when -dip1000 is passed and @system when it's not to prevent any DIP1000 regressions. #6278 Doesn't have a mechanism for that.

Copy link
Member

Choose a reason for hiding this comment

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

Argh fair enough - there still might be issues as long as -deps compiles all unittests of its dependencies :/
Anyhow, for the time being #6278 (or similar could set DIP1000 by hand).

{
@safe unittest
{
auto args = ["prog", "--foo", "-b"];

bool foo;
bool bar;
auto rslt = getopt(args, "foo|f", "Some information about foo.", &foo, "bar|b",
"Some help message about bar.", &bar);
}
}

/**
Configuration options for $(D getopt).

Expand Down Expand Up @@ -681,7 +694,7 @@ private template optionValidator(A...)

private void getoptImpl(T...)(ref string[] args, ref configuration cfg,
ref GetoptResult rslt, ref GetOptException excep,
void[][string] visitedLongOpts, void[][string] visitedShortOpts, T opts)
void[][string] visitedLongOpts, void[][string] visitedShortOpts, scope T opts)
{
enum validationMessage = optionValidator!T;
static assert(validationMessage == "", validationMessage);
Expand All @@ -700,7 +713,16 @@ private void getoptImpl(T...)(ref string[] args, ref configuration cfg,
else
{
// it's an option string
auto option = to!string(opts[0]);
static if (is(typeof(opts[0]) == string))
{
// allocate a new string so as to remove the
// scope restriction
Copy link
Member

Choose a reason for hiding this comment

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

That really sucks. Safety shouldn't result in a performance overhead :/
Especially because in this case

  • getoptImpl doesn't store anything, but modifies configuration and rslt
  • it's a GC allocated string

string option = opts[0].idup;
}
else
{
string option = to!string(opts[0]);
}
if (option.length == 0)
{
excep = new GetOptException("An option name may not be an empty string", excep);
Expand Down Expand Up @@ -729,7 +751,9 @@ private void getoptImpl(T...)(ref string[] args, ref configuration cfg,
static if (is(typeof(opts[1]) : string))
{
auto receiver = opts[2];
optionHelp.help = opts[1];
// allocate a new string so as to remove the
// scope restriction
optionHelp.help = opts[1].idup;
immutable lowSliceIdx = 3;
}
else
Expand Down Expand Up @@ -802,7 +826,7 @@ private void getoptImpl(T...)(ref string[] args, ref configuration cfg,
}
}

private bool handleOption(R)(string option, R receiver, ref string[] args,
private bool handleOption(R)(string option, scope R receiver, ref string[] args,
ref configuration cfg, bool incremental)
{
import std.algorithm.iteration : map, splitter;
Expand Down