Skip to content

add std.array.pointer as @safe alternative to .ptr#6231

Closed
timotheecour wants to merge 1 commit intodlang:masterfrom
timotheecour:pr_pointer
Closed

add std.array.pointer as @safe alternative to .ptr#6231
timotheecour wants to merge 1 commit intodlang:masterfrom
timotheecour:pr_pointer

Conversation

@timotheecour
Copy link
Contributor

@timotheecour timotheecour commented Feb 27, 2018

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 27, 2018

Thanks for your pull request and interest in making D better, @timotheecour! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.


/++
Return `.ptr` or null for an array, mitigating the fact we can't use `.ptr` in @safe code.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add an explanatory note that a.ptr could be an invalid but non-null address if a was produced by slicing another array a = b[$ .. $], so it's clear to readers that this function is doing something that might prevent memory corruption and isn't just jumping through hoops to appease the compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

std/array.d Outdated

/++
Return `.ptr` or `null` for an array `a`, mitigating the fact we can't use `.ptr` in @safe code.
`a.ptr` could be an invalid but non-null address if `a` was produced as en empty
Copy link
Member

Choose a reason for hiding this comment

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

"en" -> "an"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@schveiguy
Copy link
Member

should this target stable

No, stable shouldn't get new features.

I'm OK with this addition. Ping @andralex for feedback on adding this one-liner. While it's trivial and easy to do with a ?: operator, a developer might appreciate a wrapper for this as a drop-in replacement for .ptr in his safe code.

@JackStouffer
Copy link
Contributor

I don't think this is behavior we should encourage. In pretty much all cases, you'd want to avoid .ptr all together in @safe code. The printf example in the bug report seems contrived because printf isn't @safe anyway.

@schveiguy
Copy link
Member

@JackStouffer the impetus for this change was from here: https://forum.dlang.org/post/lqvihadaufwivbiigtqv@forum.dlang.org

In any case, I'm also fine rejecting this, but it's at least correctly @safe.

@n8sh
Copy link
Member

n8sh commented Feb 27, 2018

I don't think this is behavior we should encourage.

Even in non-safe code this is a better way to get the pointer for an array of unknown provenance.

@JackStouffer
Copy link
Contributor

https://forum.dlang.org/post/lqvihadaufwivbiigtqv@forum.dlang.org

TBH this also seems like a contrived example. How often are C functions which take pointers marked as @safe?

@schveiguy
Copy link
Member

It's definitely a contrived example, fun isn't a real function!

But any time you want to pass a value by reference, it's going to be a pointer in C. This could be perfectly safe. I've seen many functions that have parameters of "pass in a pointer to X if you want it filled. If you pass in NULL the value is ignored".

@schveiguy
Copy link
Member

BTW, I agree this use is of very limited value. Even though I'm arguing that there are some use cases, and that it is indeed valid @safe code, I'm not saying it's important to get it into phobos. Certainly, people can write the arr.length ? &arr[0] : null thing directly.

@timotheecour
Copy link
Contributor Author

timotheecour commented Feb 27, 2018

arr.length ? &arr[0] : null

actually this'll lead to less efficient code compared to this PR (unless -noboundscheck is passed) so the alternative is not so nice

@timotheecour timotheecour changed the title add std.array.pointer to mitigate .ptr deprecation add std.array.pointer as @safe alternative to .ptr Feb 27, 2018
@schveiguy
Copy link
Member

actually this'll lead to less efficient code compared to this PR

Yes, I know. But it doesn't have to, the compiler could easily elide the bounds check in that expression.

@WalterBright
Copy link
Member

WalterBright commented Feb 28, 2018

This pr is a strong no from me, but I'll leave it open for a while so everyone can have a say on it. As @schveiguy posted earlier, the idea is to bounds check &array[0] so the pointer points to valid data. I don't think this addition to std.array adds much of any value.

@andralex
Copy link
Member

We don't need this kind of one-liners catering to rare cases. The code can be inlined as needed.

@schveiguy
Copy link
Member

With a strong "no" from both the big guys, I think we can safely close this. Good news is, it's easy to do something like this in your project if you want it.

@WalterBright would it be worth opening a bugzilla request for the bounds check to be elided on arr.length > 0 ? &arr[0] : null ?

@schveiguy schveiguy closed this Feb 28, 2018
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.

8 participants