Skip to content

Translate _d_arrayassign{,_l,_r} to templates#14310

Merged
RazvanN7 merged 1 commit intodlang:masterfrom
teodutu:template-_d_arrayassign
Aug 2, 2022
Merged

Translate _d_arrayassign{,_l,_r} to templates#14310
RazvanN7 merged 1 commit intodlang:masterfrom
teodutu:template-_d_arrayassign

Conversation

@teodutu
Copy link
Member

@teodutu teodutu commented Jul 16, 2022

  • Implement template _d_arrayassign{_l,_r}.
  • Lower array assignment expressions to the above templates.
  • Remove old lowering from e2ir.d.
  • Remove the old _d_arrayassign_{l,r} and _d_arrayassig as its usage was merged with _d_arrayassign_l.

@teodutu teodutu requested a review from ibuclaw as a code owner July 16, 2022 23:30
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @teodutu! 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

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.

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 + dmd#14310"

@teodutu teodutu force-pushed the template-_d_arrayassign branch from 72cbfbd to 088579a Compare July 17, 2022 06:04
@teodutu teodutu requested a review from thewilsonator July 17, 2022 06:05
@teodutu teodutu force-pushed the template-_d_arrayassign branch from 088579a to 4e1be99 Compare July 17, 2022 06:20
@thewilsonator
Copy link
Contributor

Could you please double check that the build kite failures are unrelated? Otherwise this looks good. Please ping me when you're done.

- Implement template `_d_arrayassign{_l,_r}`
- Lower array asignment expressions to the above templates
- Remove old lowering from e2ir.d
- Remove the old `_d_arrayassign{,_l,_r}` hooks
- Merge the usage of `_d_arrayassign` with that of `_d_arrayassign_l`

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
@teodutu teodutu force-pushed the template-_d_arrayassign branch from 4e1be99 to 9d3e496 Compare July 28, 2022 13:23
@teodutu
Copy link
Member Author

teodutu commented Jul 28, 2022

@thewilsonator The buildkite failures were related. They were caused by the temporary variable used for _d_arrayappend_r:

_tmp = rhs, _d_arrayappend_r(lhs, _tmp);

I was not adding the maybescope storage class to _tmp so the compiler believed a scope rhs was ecaping. This caused both buildkite failures. They should work fine now.

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.

LGTM!

@RazvanN7
Copy link
Contributor

cc @dkorpel

@RazvanN7 RazvanN7 requested a review from dkorpel July 28, 2022 14:03
@RazvanN7 RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jul 28, 2022
@RazvanN7 RazvanN7 merged commit c83dff2 into dlang:master Aug 2, 2022
@teodutu teodutu deleted the template-_d_arrayassign branch August 22, 2022 21:00
@schveiguy
Copy link
Member

This change introduced unnecessary out-of-bounds checking when doing array operations. e.g.: https://github.com/dlang/dmd/pull/14310/files#diff-c9a721f453e1e17d997f8603ca3e4e65c765f0ebf0dd1cfb972ddddc959bdbb1R92-R94

@teodutu
Copy link
Member Author

teodutu commented Dec 5, 2022

@schveiguy Are these checks relevant given that they're disabled when compiling with maximum optimisations, i.e. -O -release -inline -boundscheck=off?

@schveiguy
Copy link
Member

Yes, they are relevant. First, because they are redundant, the entire operation is bounds checked first (see here)

And second, because it's a template, it will be compiled under whatever options the user is using at the time.

@CyberShadow
Copy link
Member

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=23959

nybzmr added a commit to nybzmr/dmd that referenced this pull request Mar 19, 2025
thewilsonator pushed a commit that referenced this pull request Mar 19, 2025
* Remove RTLSYM for Translation PR #15819: Removed NEWARRAYMITX, NEWARRAYMITX, TRACENEWARRAYMTX and TRACENEWARRAYMITX

* Remove RTLSYM for Translation PR #15299: Removed NEWARRAYT, NEWARRAYIT, TRACENEWARRAYT and TRACENEWARRAYIT

* Remove RTLSYM for Translation PR #14837: Removed NEWCLASS, TRACENEWCLASS

* Remove RTLSYM for Translation PR #14664: Removed NEWITEMT, NEWITEMIT, TRACENEWITEMT and TRACENEWITEMIT

* Remove RTLSYM for Translation PR #14550: Removed ARRAYCATNTX, ARRAYCATT, TRACEARRAYCATNTX and TRACEARRAYCATT

* Remove RTLSYM for Translation PR #14382: Removed ARRAYSETASSIGN

* Remove RTLSYM for Translation PR #14310: Removed ARRAYASSIGN

* Remove RTLSYM for Translation PR #13495: Removed ARRAYAPPENDT, ARRAYAPPENDCTX, TRACEARRAYAPPENDT and TRACEARRAYAPPENDCTX
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merge:72h no objection -> merge The PR will be merged if there are no objections raised.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants