Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

Fix Issue 14439 - aa's keys, values not usable in @safe context #3528

Merged
dlang-bot merged 1 commit intodlang:masterfrom
nordlow:trusted-AA.values
Aug 5, 2021
Merged

Fix Issue 14439 - aa's keys, values not usable in @safe context #3528
dlang-bot merged 1 commit intodlang:masterfrom
nordlow:trusted-AA.values

Conversation

@nordlow
Copy link
Contributor

@nordlow nordlow commented Jul 30, 2021 •
edited
Loading

Added unittest that verifies that safety is inferred from safety of Key/Value.this(this) when present. When postblit is not present AA.keys/values should be unconditionally @safe.

Closes https://issues.dlang.org/show_bug.cgi?id=14439.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jul 30, 2021 •
edited
Loading

Thanks for your pull request and interest in making D better, @nordlow! 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 coverage diff by visiting the details link of the codecov check)
  • 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

Auto-close Bugzilla Severity Description
✓ 14439 major aa's keys, values not usable in @safe context

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3528"

@nordlow nordlow force-pushed the trusted-AA.values branch from 367e85b to 6e39843 Compare July 30, 2021 11:32
nordlow
nordlow commented Jul 30, 2021
View reviewed changes
@nordlow nordlow changed the title Make AA.values @safe when postblit is @safe Draft: Make AA.values @safe when postblit is @safe Jul 30, 2021
@nordlow nordlow changed the title Draft: Make AA.values @safe when postblit is @safe Draft: Infer @safety of AA.values from Value.this(this) Jul 30, 2021
@nordlow nordlow changed the title Draft: Infer @safety of AA.values from Value.this(this) Draft: Infer @safety of AA.values from AA.Value.this(this) Jul 30, 2021
@nordlow nordlow force-pushed the trusted-AA.values branch from 6e39843 to 9fa310e Compare July 30, 2021 18:16
@nordlow nordlow changed the title Draft: Infer @safety of AA.values from AA.Value.this(this) Infer @safety of AA.values from AA.Value.this(this) Jul 30, 2021
@nordlow
Copy link
Contributor Author

nordlow commented Jul 30, 2021 •
edited
Loading

Ready for review now. make -f posix.mak unittest passes locally. Ping, @RazvanN7

@nordlow nordlow force-pushed the trusted-AA.values branch from 9fa310e to bbc562a Compare July 30, 2021 21:54
@nordlow nordlow changed the title Infer @safety of AA.values from AA.Value.this(this) Deduce safety of AA.keys/values from safety of Key/Value postblit Jul 30, 2021
@nordlow nordlow changed the title Deduce safety of AA.keys/values from safety of Key/Value postblit Infer safety of AA.keys/values from safety of Key/Value postblit Jul 30, 2021
RazvanN7
RazvanN7 suggested changes Aug 1, 2021
View reviewed changes
Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Needed:

  • referenced bugzilla entry
  • a test with a @System postblit
  • what about copy constructors?

nordlow
nordlow commented Aug 1, 2021
View reviewed changes
nordlow
nordlow commented Aug 1, 2021
View reviewed changes
@nordlow nordlow changed the title Infer safety of AA.keys/values from safety of Key/Value postblit Fix Issue 14439 - aa's keys, values not usable in @safe context Aug 2, 2021
@nordlow nordlow changed the title Fix Issue 14439 - aa's keys, values not usable in @safe context Fix Issue 14439:aa's keys, values not usable in @safe context Aug 2, 2021
@nordlow nordlow changed the title Fix Issue 14439:aa's keys, values not usable in @safe context Fix Issue 14439: aa's keys, values not usable in @safe context Aug 2, 2021
@nordlow nordlow changed the title Fix Issue 14439: aa's keys, values not usable in @safe context Fix Issue 14439 - aa's keys, values not usable in @safe context Aug 2, 2021
@nordlow
Copy link
Contributor Author

nordlow commented Aug 2, 2021 •
edited
Loading

Added link to bugzilla issue. There's already #1644 but I won't have time to look into all the details of that PR. Ok, to merge this anyway, @RazvanN7 ?

@RazvanN7
Copy link
Contributor

RazvanN7 commented Aug 2, 2021

You still need a commit that contains "Fix Issue 14439" so that the bot can pick it up.

@nordlow nordlow force-pushed the trusted-AA.values branch from bbc562a to b5563be Compare August 2, 2021 12:00
@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Aug 2, 2021
@nordlow
Copy link
Contributor Author

nordlow commented Aug 2, 2021 •
edited
Loading

You still need a commit that contains "Fix Issue 14439" so that the bot can pick it up.

Done. Made commit message equal PR title. Ready now, @RazvanN7

RazvanN7
RazvanN7 approved these changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks!

@RazvanN7 RazvanN7 mentioned this pull request Aug 2, 2021
Closed
@RazvanN7
Copy link
Contributor

RazvanN7 commented Aug 2, 2021

A rebase is probably going to fix the existing failures

@RazvanN7 RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Aug 2, 2021
andralex
andralex reviewed Aug 2, 2021
View reviewed changes
Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

Thanks for the work!

@nordlow nordlow force-pushed the trusted-AA.values branch 2 times, most recently from 9aef264 to e970797 Compare August 2, 2021 21:37
@nordlow
Copy link
Contributor Author

nordlow commented Aug 2, 2021 •
edited
Loading

Adjusted for @andralex's comments and rebased. Unittests pass locally. Ready again, @RazvanN7

@RazvanN7
Copy link
Contributor

RazvanN7 commented Aug 3, 2021

@nordlow I'll give it another 2 days until the 72h->merge label expires so that others have a chance to take look.

@RazvanN7 RazvanN7 added auto-merge and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels Aug 5, 2021
@dlang-bot dlang-bot merged commit 165499e into dlang:master Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MartinNowak MartinNowak Awaiting requested review from MartinNowak

3 more reviewers

@andralex andralex andralex left review comments

@skoppe skoppe skoppe left review comments

@RazvanN7 RazvanN7 RazvanN7 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug Fix Include reference to corresponding bugzilla issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nordlow @dlang-bot @RazvanN7 @andralex @skoppe