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.

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

Jump to bottom

Turn .ptr deprecation into an error #7688

Merged
merged 1 commit into from
Jan 14, 2018
Merged

Turn .ptr deprecation into an error #7688

merged 1 commit into from
Jan 14, 2018

Conversation

wilzbach
Copy link
Member

This was scheduled to happen in 2.073 - we really need a file or wiki to list
these things, s.t. they aren't forgotten.

I'm abusing auto-tester for testing as due to -fPIC I still
can't run the testsuite locally.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

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.

@wilzbach wilzbach added the WIP Work In Progress - not ready for review or pulling label Jan 12, 2018
@wilzbach wilzbach mentioned this pull request Jan 12, 2018
Merged
@quickfur
Copy link
Member

LGTM

@ibuclaw
Copy link
Member

ibuclaw commented Jan 12, 2018

finger hovers on merge button

JinShil
JinShil previously approved these changes Jan 13, 2018
View reviewed changes
@JinShil
Copy link
Contributor

JinShil commented Jan 13, 2018

Why is this labeled "WIP"?

@ibuclaw
Copy link
Member

ibuclaw commented Jan 13, 2018

I assume because @wilzbach can't test locally, so using ci to tell him/us what breaks with this change.

I'll block on missing changelog anyhow.

ibuclaw
ibuclaw requested changes Jan 13, 2018
View reviewed changes
Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

Changelog entry

@JinShil JinShil added the Needs Changelog A changelog entry needs to be added to /changelog label Jan 13, 2018
@JinShil
Copy link
Contributor

JinShil commented Jan 13, 2018

Curious that this didn't require a change to any test cases, yet coverage shows it's covered. 🤔

@ibuclaw
Copy link
Member

ibuclaw commented Jan 13, 2018

The tests are likely fail_compilation with -de. They don't need that switch anymore.

@wilzbach wilzbach removed the Needs Changelog A changelog entry needs to be added to /changelog label Jan 13, 2018
@wilzbach
Copy link
Member Author

I assume because @wilzbach can't test locally, so using ci to tell him/us what breaks with this change.

Yeah, I often patch the test suite, but sometimes I'm in a hurry.
(reminder to myself: I should really look into getting the -fPIC PRs merged)

Curious that this didn't require a change to any test cases, yet coverage shows it's covered. 🤔

Found the test.

I'll block on missing changelog anyhow.

Added.
(Just in case someone didn't know this, the rendered changelog can be previewed at DAutoTest.)

@wilzbach wilzbach removed the WIP Work In Progress - not ready for review or pulling label Jan 13, 2018
@wilzbach wilzbach closed this Jan 13, 2018
@wilzbach wilzbach reopened this Jan 13, 2018
@jacob-carlborg
Copy link
Contributor

.ptr can still be used in @system code? If that's the case, is it worth mentioning explicitly in the changelog?

@wilzbach
Copy link
Member Author

If that's the case, is it worth mentioning explicitly in the changelog?

I didn't judge it as important enough, because it's more or less obvious, but that you are wondering about it, is reason enough to include it (I just did so).

JinShil
JinShil reviewed Jan 13, 2018
View reviewed changes
test/fail_compilation/test11176.d Outdated
@@ -1,5 +1,5 @@
/*
REQUIRED_ARGS: -de
REQUIRED_ARGS:
---
fail_compilation/test11176.d(12): Deprecation: b.ptr cannot be used in @safe code, use &b[0] instead
fail_compilation/test11176.d(16): Deprecation: b.ptr cannot be used in @safe code, use &b[0] instead
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't making any sense to me. Why are the tests passing? The output of this test should be

Error: `b.ptr` cannot be used in `@safe` code, use `&b[0]` instead

Copy link
Member

Choose a reason for hiding this comment

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

There is no TEST_OUTPUT in the comment, and so the output is not tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent catch - the test helped me to realize that there was another Deprecation message that needed maintenance (i.e. the one for dynamic arrays).

@JinShil JinShil dismissed their stale review January 13, 2018 11:41

Somp'n wrong here

@ibuclaw
Copy link
Member

ibuclaw commented Jan 13, 2018

I'm going to say all good to go. @JinShil ?

ibuclaw
ibuclaw requested changes Jan 13, 2018
View reviewed changes
src/dmd/mtype.d Outdated
e.deprecation("`%s.ptr` cannot be used in `@safe` code, use `&%s[0]` instead", e.toChars(), e.toChars());
// return new ErrorExp();
e.error("`%s.ptr` cannot be used in `@safe` code, use `&%s[0]` instead", e.toChars(), e.toChars());
return new ErrorExp();
Copy link
Member

Choose a reason for hiding this comment

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

Funny whitespace

JinShil
JinShil approved these changes Jan 13, 2018
View reviewed changes
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.

Just waiting on whitespace fix. I really need to figure out how to do such changes as part of reviewing/merging.

@JinShil
Copy link
Contributor

JinShil commented Jan 14, 2018

Fixed whitespace with pr_push. Thanks @wilzbach!

@dlang-bot dlang-bot merged commit 5ff13b9 into dlang:master Jan 14, 2018
@wilzbach wilzbach deleted the maintainence branch January 14, 2018 04:49
@wilzbach wilzbach mentioned this pull request Jan 14, 2018
Closed
@wilzbach wilzbach mentioned this pull request Jan 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibuclaw ibuclaw ibuclaw requested changes

@JinShil JinShil JinShil approved these changes

Assignees
No one assigned
Labels
auto-merge
Projects
None yet
Milestone
No milestone
6 participants
@wilzbach @dlang-bot @quickfur @ibuclaw @JinShil @jacob-carlborg