Skip to content

Conversation

@JackStouffer
Copy link
Contributor

No description provided.

@JackStouffer JackStouffer requested a review from andralex as a code owner March 15, 2018 18:14
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @JackStouffer!

Bugzilla references

Auto-close Bugzilla Severity Description
18172 enhancement std.getopt should allow taking parameters by ref (like std.format.formattedRead), s.t. it can be used in @safe

@JackStouffer JackStouffer changed the title Work On Issue 18172 - add scope to pointers to allow @safe Work On Issue 18172 - in getopt add scope to pointers to allow @safe Mar 15, 2018
}
}

version(DIP1000)
Copy link
Contributor 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
Contributor

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
Contributor 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
Contributor

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).

std/getopt.d Outdated
{
// allocate a new string so as to remove the
// scope restriction
string option = "" ~ opts[0];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is due to the fact that to!(string)(string) acts as a pass through.

Copy link
Contributor

Choose a reason for hiding this comment

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

.idup?

@JackStouffer JackStouffer changed the title Work On Issue 18172 - in getopt add scope to pointers to allow @safe Work On Issue 18172 - add scope to pointers in getopt to allow @safe Mar 15, 2018
static if (is(typeof(opts[0]) == string))
{
// allocate a new string so as to remove the
// scope restriction
Copy link
Contributor

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

@wilzbach
Copy link
Contributor

wilzbach commented Mar 22, 2018

Here's an idea how to programmatically detect -dip1000:

void main()
{
    enum isDIP1000 = __traits(compiles, () @safe {
         int x;
         int* p;
         p = &x;
    });
    pragma(msg, isDIP1000);
}

normal: https://run.dlang.io/is/RID7vh
-dip1000: https://run.dlang.io/is/1yJfVQ

Of course that could be fancier.

(based on the recent forum thread: https://forum.dlang.org/post/ayemvxctmpqqsokbmqeb@forum.dlang.org)

@JackStouffer
Copy link
Contributor Author

If people want to tackle the DIP1000 returning a struct issue, I'd be glad to see their solution. Until then, I'm closing this as this isn't going to go anywhere.

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.

4 participants