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

Add RequireMutable #3391

Merged
merged 1 commit into from
Apr 7, 2019
Merged

Conversation

ChrisJefferson
Copy link
Contributor

This adds the C function "RequireMutable".

This took more work than I planned, as I found errors of the type:

" must be mutable (not a ...)" a bit confusing, as for this error I found the "not a ..." bits misleading.

@coveralls
Copy link

coveralls commented Apr 6, 2019

Coverage Status

Coverage increased (+0.004%) to 85.158% when pulling 3cdf2c8 on ChrisJefferson:require_mutable into 6f1661e on gap-system:master.

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.

I like this PR and the fact that it simplifies the code.

I asked one question about the removed ErrorReturnVoid, I just want to make sure it was a deliberate decision to change that behaviour, if so I’m happy to approve.

My only complaint is that the error messages have less information, but then again the error that is given corresponds exactly to the condition that is being tested. So fair enough.

src/vecgf2.c Show resolved Hide resolved
tst/testinstall/listindex.tst Outdated Show resolved Hide resolved
src/error.h Outdated Show resolved Hide resolved
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Generally a good thing! But some remarks (inspired by @wilfwilson )

src/listfunc.c Outdated Show resolved Hide resolved
src/error.c Outdated Show resolved Hide resolved
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.

Given that @fingolfin explained my question to me, I approve the PR; if you want to add more information into the error messages, as discussed in the thread, then that would be welcome, but I don't insist on it.

@wilfwilson wilfwilson added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: error handling 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 7, 2019

Codecov Report

Merging #3391 into master will increase coverage by 0.35%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #3391      +/-   ##
==========================================
+ Coverage   85.15%    85.5%   +0.35%     
==========================================
  Files         697      646      -51     
  Lines      344073   317133   -26940     
==========================================
- Hits       292998   271168   -21830     
+ Misses      51075    45965    -5110
Impacted Files Coverage Δ
src/error.h 88.88% <ø> (-11.12%) ⬇️
src/lists.c 75% <100%> (-0.17%) ⬇️
src/listfunc.c 93.47% <100%> (-0.61%) ⬇️
src/vecgf2.c 72.6% <62.5%> (-1.12%) ⬇️
lib/random.g 66.66% <0%> (-30.56%) ⬇️
lib/read2.g 85.71% <0%> (-14.29%) ⬇️
lib/read1.g 85.36% <0%> (-13.42%) ⬇️
lib/session.g 57.69% <0%> (-12.83%) ⬇️
src/calls.h 87.59% <0%> (-10.29%) ⬇️
... and 173 more

@ChrisJefferson
Copy link
Contributor Author

ChrisJefferson commented Apr 7, 2019

This patch got massively simplified :) But I think the messages are much better. Thanks for all feedback.

I did keep the patch to remove the extra message, but I feel once we say "must be a mutable list", it's fine to then say "(not a ...)"

@ChrisJefferson ChrisJefferson force-pushed the require_mutable branch 2 times, most recently from 151e034 to 64d7b53 Compare April 7, 2019 14:06
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Nice! One minor remark, but merging as-is would be fine as well!

src/vecgf2.c Outdated Show resolved Hide resolved
@wilfwilson wilfwilson merged commit 3d6f4d7 into gap-system:master Apr 7, 2019
@ChrisJefferson ChrisJefferson deleted the require_mutable branch May 8, 2019 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: error handling topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants