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.

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

Cleanup to lsra inspired by #73424 #73589

Closed
wants to merge 5 commits into from
Closed

Conversation

markples
Copy link
Member

@markples markples commented Aug 8, 2022

Inspired by comments (here and here)

  • Remove RefPosition* currentRefPosition = &reverseIterator as it creates two variables that are essentially the same, especially since the iterator has operators like ->. In fact, I had to spend a bit of time verifying that there weren't any updates to one but not the other in the code. (And actually Fix undefined behavior found with g++-12 ubsan #73424 did slightly impact this relationship since it moved the assignment from the loop iteration step to the loop body, but I don't believe that this mattered.)
    I renamed the "iterator" variable to the "position" name to reduce textual churn in the code. This didn't work as well for the range-based loops since they yield -references- to the underlying object so a bunch of C++ punctuation changes.
  • Break apart two complicated for conditions and remove duplication between the condition and the loop body. Search for continueLoop to see them.
  • Delete list's operator& on each iterator since they are no longer used and not part of normal iterators.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 8, 2022
@dnfadmin
Copy link

dnfadmin commented Aug 8, 2022

CLA assistant check
All CLA requirements met.

@ghost ghost assigned markples Aug 8, 2022
@ghost
Copy link

ghost commented Aug 8, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Inspired by comments (here and here)

  • Remove RefPosition* currentRefPosition = &reverseIterator as it creates two variables that are essentially the same, especially since the iterator has operators like ->. In fact, I had to spend a bit of time verifying that there weren't any updates to one but not the other in the code. (And actually Fix undefined behavior found with g++-12 ubsan #73424 did slightly impact this relationship since it moved the assignment from the loop iteration step to the loop body, but I don't believe that this mattered.)
    I renamed the "iterator" variable to the "position" name to reduce textual churn in the code. This didn't work as well for the range-based loops since they yield -references- to the underlying object so a bunch of C++ punctuation changes.
  • Break apart two complicated for conditions and remove duplication between the condition and the loop body. Search for continueLoop to see them.
Author: markples
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member

It might be nice to remove the operator& overloads on the various iterator classes as part of this cleanup (I don't know why we have this, it is not part of iterators in the STL). I think you can mark them deleted as an intermediate step to make sure there are no remaining uses of them.

@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.0 milestone Aug 8, 2022
@markples
Copy link
Member Author

markples commented Aug 9, 2022

It might be nice to remove the operator& overloads [...]

It built fine on the first try - thanks for the recommendation.

@ghost
Copy link

ghost commented Sep 8, 2022

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 8, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants