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

Make structural copy handle Blists properly. #1514

Merged
merged 2 commits into from
Jul 20, 2017

Conversation

markuspf
Copy link
Member

This is a rebased and updated version of #1455.

Sorry I couldn't keep my fingers off the clang-format button.

@codecov
Copy link

codecov bot commented Jul 17, 2017

Codecov Report

Merging #1514 into master will increase coverage by <.01%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##           master    #1514      +/-   ##
==========================================
+ Coverage   64.19%   64.19%   +<.01%     
==========================================
  Files         992      992              
  Lines      322094   322105      +11     
  Branches    13072    13071       -1     
==========================================
+ Hits       206755   206772      +17     
+ Misses     112521   112516       -5     
+ Partials     2818     2817       -1
Impacted Files Coverage Δ
src/blister.c 76.67% <84.21%> (+0.71%) ⬆️
src/objset.c 93.04% <0%> (-0.27%) ⬇️
src/stats.c 72.53% <0%> (ø) ⬆️
src/funcs.c 69.94% <0%> (+0.14%) ⬆️
src/listfunc.c 77.12% <0%> (+0.17%) ⬆️
src/hpc/threadapi.c 31.69% <0%> (+0.19%) ⬆️

@fingolfin
Copy link
Member

This looks good now. Hmm... perhaps add a test case, based on #1428 ? (I'd just add a test file for StructuralCopy and copy the test cases from the issue into it, adjusted. We can add more StructuralCopy tests in the future.

@markuspf
Copy link
Member Author

Tests added.

/* don't change immutable objects */
if ( ! IS_MUTABLE_OBJ(list) ) {
return list;
}
Copy link
Member

Choose a reason for hiding this comment

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

Huh... Hadn't thought about this before, but shouldn't this check for (im)mutable stay in? It makes sense to me from a logical point of view, and I just checked, this is also what we do for plists. @stevelinton any reason why your original PR removed this?!?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the code in again as to make this sensibly mergeable, because I agree that the check should be there. If @stevelinton comments and gives a reason why it can go, I can remove this again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because immutable Blists now have their own function entered appropriately in the dispatch table. Perhaps assert(IS_MUTABLE_OBJ(list)); should be added here.

@@ -0,0 +1,23 @@
# Blist
gap> a:=[true];; IsBlistRep(a);
Copy link
Member

Choose a reason for hiding this comment

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

Add START_TEST / STOP_TEST to this .tst file, like in the other files in that dir?

@markuspf markuspf force-pushed the pr-1455-update branch 2 times, most recently from 8a947b4 to 4dd9038 Compare July 17, 2017 18:46
@markuspf markuspf force-pushed the pr-1455-update branch 3 times, most recently from 6d4b36c to acad414 Compare July 18, 2017 19:22
@markuspf
Copy link
Member Author

I removed the check for !IS_MUTABLE_OBJ again and added a GAP_ASSERT(IS_MUTABLE_OBJ(list)). While I was there I also added a GAP_ASSERT(!IS_MUTABLE_OBJ(list)) into the handler for immutable blists.

Therefore, when tests pass we should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants