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

Prevent blist functions from modifying immutable blists (and several other kernel tweaks leading up to this) #3392

Merged
merged 6 commits into from
Apr 10, 2019
Merged

Prevent blist functions from modifying immutable blists (and several other kernel tweaks leading up to this) #3392

merged 6 commits into from
Apr 10, 2019

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Apr 6, 2019 •
edited
Loading

This conflicts with PR #3385, so that PR should be merged first; after which I can rebase and update this PR.

But I wanted to put it out now, to make sure I don't leave it bitrotting here for another half year ;-).

Resolves #3393

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Apr 6, 2019
@codecov
Copy link

codecov bot commented Apr 6, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@27c845b). Click here to learn what that means.
The diff coverage is 7.86%.

@@            Coverage Diff            @@
##             master    #3392   +/-   ##
=========================================
  Coverage          ?   69.24%           
=========================================
  Files             ?      633           
  Lines             ?   305975           
  Branches          ?        0           
=========================================
  Hits              ?   211859           
  Misses            ?    94116           
  Partials          ?        0
Impacted Files Coverage Δ
src/lists.c 42.38% <0%> (ø)
src/error.c 14.52% <0%> (ø)
src/intrprtr.c 70.72% <0%> (ø)
lib/list.g 100% <100%> (ø)
src/vars.c 43.59% <100%> (ø)
src/blister.c 31.29% <3.75%> (ø)

@coveralls
Copy link

coveralls commented Apr 6, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.0003%) to 85.159% when pulling ca9b062 on fingolfin:mh/blister-check into 27c845b on gap-system:master.

wilfwilson
wilfwilson approved these changes Apr 6, 2019
View reviewed changes
Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

This looks good to me. I appreciated the separation in to four commits, it made reviewing it easy, thanks.

@wilfwilson wilfwilson added status: awaiting a different PR PRs that should not be merged until (at least one) other PR is merged do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) labels Apr 6, 2019
@wilfwilson
Copy link
Member

I'll add the do not merge label and the new status: awaiting a different PR label until #3385 is merged. Once that happens and you've updated, I'll be happy to review again if you want.

@wilfwilson wilfwilson removed do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) status: awaiting a different PR PRs that should not be merged until (at least one) other PR is merged labels Apr 9, 2019
@wilfwilson
Copy link
Member

#3385 is now merged, so there's nothing stopping this PR from taking its purest form 🙂

fingolfin added 4 commits April 9, 2019 12:50
@fingolfin
kernel: blister.c: rename list1 to blist1
8294519
@fingolfin
kernel: <positions> -> <poss> in some errors messages
9bd1a12 
The manual entry for e.g. `{}` refers to <poss>, as does the kernel
code itself, so it seems sensible to adjust the error messages.
@fingolfin
kernel: CheckSameLength -> RequireSameLength
9984e68 
... where possible
@fingolfin
kernel: remove redundant RequireBlist arg
4ac0ee7
@fingolfin fingolfin changed the title kernel: CheckSameLength -> RequireSameLength and some changes leading up to that kernel: blist tweaks: CheckSameLength -> RequireSameLength; add mutability checks; update some errors; ... Apr 9, 2019
@fingolfin
Copy link
Member Author

I have now rebased this PR. I am not sure about the "purest form", though, it now also adds the mutability checks... I tried to break everything into "clean" commits, but let me know if you think it is too much and I should split it into two or more PRs.

wilfwilson
wilfwilson approved these changes Apr 9, 2019
View reviewed changes
Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

The separate commits make it easy to review, which I've done again, so I'm not bothered by the PR containing a few different types of changes.

If you think this resolves #3393 (which it seems to), then could you please make it so that merging this PR will close that issue?

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Apr 9, 2019
@fingolfin fingolfin changed the title kernel: blist tweaks: CheckSameLength -> RequireSameLength; add mutability checks; update some errors; ... Prevent blist functions from modifying immutable blists (and several other kernel tweaks leading up to this) Apr 9, 2019
@fingolfin fingolfin merged commit c920598 into gap-system:master Apr 10, 2019
@wilfwilson wilfwilson mentioned this pull request Apr 10, 2019
Closed
@fingolfin fingolfin deleted the mh/blister-check branch April 11, 2019 13:52
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Apr 15, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
@wilfwilson wilfwilson mentioned this pull request Jun 11, 2020
Merged
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilfwilson wilfwilson wilfwilson approved these changes

Assignees
No one assigned
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Milestone
GAP 4.11.0
Development

Successfully merging this pull request may close these issues.

Some functions in blister.c ignore mutability
4 participants
@fingolfin @coveralls @wilfwilson @olexandr-konovalov