Make structural copy handle Blists properly. #1514
Conversation
Codecov Report
@@ 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
|
|
This looks good now. Hmm... perhaps add a test case, based on #1428 ? (I'd just add a test file for |
|
Tests added. |
| /* don't change immutable objects */ | ||
| if ( ! IS_MUTABLE_OBJ(list) ) { | ||
| return list; | ||
| } |
There was a problem hiding this comment.
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?!?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
Add START_TEST / STOP_TEST to this .tst file, like in the other files in that dir?
markuspf
force-pushed
the
pr-1455-update
branch
2 times, most recently
from
8a947b4
to
4dd9038
Compare
markuspf
force-pushed
the
pr-1455-update
branch
3 times, most recently
from
6d4b36c
to
acad414
Compare
|
I removed the check for Therefore, when tests pass we should be good to go. |
olexandr-konovalov
added
the
release notes: added
This is a rebased and updated version of #1455.
Sorry I couldn't keep my fingers off the
clang-formatbutton.