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 Flip/SetAll/ClearAllBlist #3385

Merged
merged 1 commit into from
Apr 9, 2019

Conversation

james-d-mitchell
Copy link
Contributor

This PRs adds three functions for blists, FlipBlist, SetAllBitsBlist, and ClearAllBitsBlist. I found that I wanted these three functions in various pieces of code, and so I thought I'd add them. I'm not particularly attached to these particular names.

@DominikBernhardt DominikBernhardt added topic: kernel kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements labels Mar 28, 2019
@ChrisJefferson
Copy link
Contributor

The kernel documentation for blister.c says that if there are unused bits in the last word they will be set to zero. This is required for blists to work properly. This means handling the last word in flip and set will require a special case.

lib/list.g Outdated Show resolved Hide resolved
lib/list.g Outdated Show resolved Hide resolved
lib/list.g Outdated Show resolved Hide resolved
lib/list.g 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.

These new functions don't really fit into the existing blist interface at all: Just look at chapter 22 of the manual, and what functions there, and compare this with what's added in this PR: The PR exposes (already in the names of the functions) a view of blists as "lists of bits", while the rest of the documentation (and names) expose them as "list of booleans".

Thus, when regarding blists as per the documentation, the new function names make little sense. I guess ClearAllBitsBlist could be renamed to SetAllEntriesToTrue, but that would still be a rather odd function, if you think about it (at least I can't think of any similar function for other list types). The function IMHO only makes sense when one is thinking about these things in terms of bitfields. And even then: a bitfield would usually always have a size which is a multiple of 8, but more likely of 32 or 64 on modern machines (and the actual, buggy implementation in this PR, suggests to me that this is also how @james-d-mitchell is thinking about them).

I'd be somewhat less confused by a function SetBlistEntries(blist, num, bool) which sets num many entries of blist to the value given in bool. It'd still be somewhat odd, but at least would not be completely against the grain.

That said, I never liked the whole blist API. It seems to artificially cram bitfields into a GAP "list" API, but the abstraction is leaky and creaky all over the place; and we get weird functions like IntersectionBlist which has nothing to do with Intersection, but rather is a way to hide that we really want to do an "and" operation on a bitfield. The worst of them all is SizeBlist, IMHO.

Out of scope, but: Personally, what I'd really want, but probably will never get, is an actual "proper" bitfield type, which standard terminology, and always has an internal size which is a multiple of 8 or 64; and possibly even has a variant with static size. It could even internally share most of the code with blists.

lib/list.g Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

Just to clarify: in general I support the idea of adding functions with the functionality described here. What I am objecting is putting "bits" into the function names; the names should "fit" with the existing blist API.

@james-d-mitchell
Copy link
Contributor Author

As I said in my original PR I'm not particularly attached to the names, any specific suggestions?

@james-d-mitchell
Copy link
Contributor Author

I don't really agree with your comments @fingolfin regarding the "fit" of these functions, but I do agree that the names don't fit. I don't have the time to argue the point, nor do I think it will serve any purpose to do so.

I've updated the PR to:

  • set the "trailing" bits to 0, as per @ChrisJefferson 's comments.
  • fix the documentation, and the synonyms in list.g
  • the names of the functions are SetAllBlist, ClearAllBlist, and FlipBlist i.e. no mention of bits. If you prefer different names, then please let me know what they are, and I'll be happy to update the PR accordingly.

I would find these 3 functions useful, which is why I made this PR, if you decide that you don't want them in GAP, then that's ok with me, please feel free to close this PR.

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #3385 into master will decrease coverage by <.01%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master    #3385      +/-   ##
==========================================
- Coverage   85.16%   85.15%   -0.01%     
==========================================
  Files         697      697              
  Lines      344056   344109      +53     
==========================================
+ Hits       293003   293035      +32     
- Misses      51053    51074      +21
Impacted Files Coverage Δ
lib/list.g 100% <100%> (ø) ⬆️
src/blister.c 95.83% <89.74%> (-0.47%) ⬇️
src/vecgf2.c 73.72% <0%> (-0.78%) ⬇️
src/error.h 100% <0%> (ø) ⬆️
src/listfunc.c 94.08% <0%> (+0.01%) ⬆️
src/lists.c 75.16% <0%> (+0.03%) ⬆️

@ChrisJefferson
Copy link
Contributor

ChrisJefferson commented Mar 29, 2019

I wrote some more tests, I have cut+pasted them here, as that's probably the easiest way to send them to you. These currently crash. The problem is lists of length 0.

# FuncFLIP_BLIST
gap> FLIP_BLIST(fail);
Error, FlipBlist: <blist> must be a boolean list (not the value 'fail')
gap> x:= [false,true,true,false];;
gap> FLIP_BLIST(x);
gap> x;
[ true, false, false, true ]
gap> FLIP_BLIST(x);
gap> x;
[ false, true, true, false ]
gap> for i in [0..200] do
> f1 := List([1..i], x -> false);
> f2 := List([1..i], x -> false);
> t1 := List([1..i], x -> true);
> t2 := List([1..i], x -> true);
> FLIP_BLIST(f1); FLIP_BLIST(t1);
> if f1 <> t1 or f2 <> t2 then Print("Broken FLIP_BLIST\n"); fi;
> od;

# FuncSET_ALL_BLIST
gap> SET_ALL_BLIST(fail);
Error, SetAllBitsBlist: <blist> must be a boolean list (not the value 'fail')
gap> x:= [false,true,true,false];;
gap> SET_ALL_BLIST(x);
gap> x;
[ true, true, true, true ]
gap> SET_ALL_BLIST(x);
gap> x;
[ true, true, true, true ]
gap> for i in [0..200] do
> f1 := List([1..i], x -> false);
> t1 := List([1..i], x -> true);
> SET_ALL_BLIST(f1);
> if f1 <> t1 then Print("Broken SET_ALL_BLIST\n"); fi;
> od;

# FuncCLEAR_ALL_BLIST
gap> CLEAR_ALL_BLIST(fail);
Error, ClearAllBitsBlist: <blist> must be a boolean list (not the value 'fail'\
)
gap> x:= [false,true,true,false];;
gap> CLEAR_ALL_BLIST(x);
gap> x;
[ false, false, false, false ]
gap> CLEAR_ALL_BLIST(x);
gap> x;
[ false, false, false, false ]
gap> for i in [0..200] do
> f1 := List([1..i], x -> false);
> t1 := List([1..i], x -> true);
> CLEAR_ALL_BLIST(t1);
> if f1 <> t1 then Print("Broken CLEAR_ALL_BLIST\n"); fi;
> od;

@james-d-mitchell
Copy link
Contributor Author

Thanks @ChrisJefferson, I didn't account for the list of length 0 case. I don't think your tests are valid though, you flip f1 := List([1..i], x -> false); and t1 := List([1..i], x -> true);, so I wouldn't expect them to be the same. Did you mean to compare f1 and t2, and t1 and f2, instead?

@ChrisJefferson
Copy link
Contributor

Woops, yes, you are right :) That's what I was aiming for (I'm using the fact that I know that = compares the whole block of memory, to check we set/clear bits off the end correctly).

@james-d-mitchell
Copy link
Contributor Author

I've updated the PR to handle the 0 length blist case, and to add @ChrisJefferson tests.

@coveralls
Copy link

coveralls commented Mar 29, 2019

Coverage Status

Coverage increased (+0.0005%) to 85.158% when pulling eda9f39 on james-d-mitchell:blister into 3d6f4d7 on gap-system:master.

@james-d-mitchell james-d-mitchell changed the title Add Flip/Set/ClearAllBitsBlist Add Flip/SetAll/ClearAllBlist Mar 29, 2019
@james-d-mitchell
Copy link
Contributor Author

I'm going to close this because there are no comments, or further requests for changes in the past 7 days.

@ChrisJefferson
Copy link
Contributor

Obviously, you can shut the PR (I won't force reopen it), but I did generally like the work. All my issues with it were fixed, but I know some other people had issues so I was waiting to see if they commented. If you reopen, I'll accept the PR and we can merge it (unless of course someone does still have an issue).

@james-d-mitchell
Copy link
Contributor Author

I got the impression that this isn't a desirable feature in the end, I'll re-open if I got the wrong impression. If it's not though, I'll move it into one of our packages, rather than leaving it as an open PR for longer.

@wilfwilson
Copy link
Member

The only person to say anything particularly critical was @fingolfin, and even then his clarification:

Just to clarify: in general I support the idea of adding functions with the functionality described here.

was pretty clear to me. I had the impression that this would be merged, so I think you've got the wrong impression. I know it's annoying to make changes and wait a week and hear nothing, but that's the nature of GAP, where most contributors have to fit their work on GAP around (usually) much more pressing work issues.

@james-d-mitchell
Copy link
Contributor Author

@wilfwilson, I am not annoyed by waiting, nor by anything @fingolfin or @ChrisJefferson wrote. @fingolfin's first and second comments seem contradictory to me, so I wasn't sure what to think.

@wilfwilson
Copy link
Member

Fair enough, glad you weren't annoyed. @fingolfin has started at a new university this week, so it might be a bit longer before he'll comment further on this, if he intends to. I'll reopen it to give it some more time for comments, I hope you don't mind. In particular I'll most likely give it an 'Approving' review, if and when I have time 🙂

@wilfwilson wilfwilson reopened this Apr 5, 2019
@fingolfin
Copy link
Member

I was indeed (and still am) super busy with the new job, where I also had to start teaching on the very first day, organize furniture, deal with tons of bureaucracy and more.

And just to put things in perspective: I have plenty of examples where a PR of mine got no review at all for more than a month. Of course that's not ideal, but this is just what happens if all contributors essentially only can work on GAP as a side job. As such I am grateful that recently some more people started to help with reviews; before that, in the past year or so, it was mostly Chris and me who took care of the bulk of reviews.

@fingolfin
Copy link
Member

As to this:

@fingolfin's first and second comments seem contradictory to me

I am not sure what to make of that: my second comment specifically stated that it was meant to clarify the first comment, in that I pointed out that I was objecting to the fit of the names, not the fit of functions (which I had indeed phrased a bit carelessly in my initial comment). So, to me, it's pretty clear that the second comment clarifies and hence supersedes the first. I really don't know how to make it more clear.

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.

Looks good to me overall, though I have a small change request regarding whitespace.

src/blister.c Outdated Show resolved Hide resolved
src/blister.c Outdated

UInt * ptr = BLOCKS_BLIST(list);
for (UInt i = NUMBER_BLOCKS_BLIST(list); 0 < i; i--) {
*ptr++ = (UInt)~0;
Copy link
Member

Choose a reason for hiding this comment

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

Very strictly and pedantically speaking, (UInt)~0 (also elsewhere in this PR) should be ~(UInt)0 for full portability, although I assume all machines built today and probably also in the future will be using two's complement, so it probably doesn't matter... (some of the code in this file also uses ~0UL).

Anyway, I won't complain if this is left as-is, we probably rely on two's complement elsewhere anyway shrug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change this also

tst/testinstall/kernel/blister.tst Show resolved Hide resolved
@james-d-mitchell
Copy link
Contributor Author

As to this:

@fingolfin's first and second comments seem contradictory to me

I am not sure what to make of that: my second comment specifically stated that it was meant to clarify the first comment, in that I pointed out that I was objecting to the fit of the names, not the fit of functions (which I had indeed phrased a bit carelessly in my initial comment). So, to me, it's pretty clear that the second comment clarifies and hence supersedes the first. I really don't know how to make it more clear.

It wasn't clear to me if the names changes were what you were aiming at, or if you would only accept a version of the method along the lines of the SetBlistEntries that you wrote about in your first comment, or indeed if there were further issues with the PR. I thought there was a recent request to close PR/issues without comments after some time, but apparently I got that wrong.

@wilfwilson wilfwilson added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 6, 2019
@wilfwilson
Copy link
Member

wilfwilson commented Apr 6, 2019

I thought there was a recent request to close PR/issues without comments after some time, but apparently I got that wrong.

On this topic:

At the GAP days in Halle, a group of us spoke about ways that we could manage issues and PRs more effectively. The aim was to make the job easier for those who review PRs and attempt to solve issues, rather than adding hoops for PR/issue creators to jump through. I've been writing up something about this to help contributors, but I'm not finished yet.

The main thrust is to use labels more effectively, which seems to be going well in my opinion. There's no particular aim to close issues or PRs (beyond actually resolving them/merging them in the proper way), however we have introduced a small measure along these lines:

Since #3366 was merged, issues/PRs that are given the label status: awaiting response will be automatically closed after 14 days if the original author does not reply. The idea is to close issues/PRs where there is not enough information from the author to proceed, and where the author is not forthcoming with the required information (perhaps 14 days should be increased?). I think this is a fairly small use case, and so far nothing has been closed in this way (although #2386 is close).

(In particular, it wouldn't apply in cases like this PR, where although nothing happened for a period of time, it wasn't because of a lack of information).

@ChrisJefferson
Copy link
Contributor

Sorry to kick up one more problem!

This isn't checking the blists are mutable. You could either fix this with some code a bit like:

 if (!IS_MUTABLE_OBJ(list1))
        ErrorMayQuit("Append: <list1> must be a mutable", 0, 0);

Or, wait until (hopefully) #3391 is merged, at which point you will be able to write:

RequireMutable(list1);

@fingolfin
Copy link
Member

@ChrisJefferson good catch on the mutability -- but to be fair, most other functions in this file also lack this :-/, e.g. FuncUNITE_BLIST. So I'd also be fine with accepting this "bug" in this PR for now, and then follow it up soon with another PR which adds the missing RequireMutable calls to all blister.c functions, together with test cases. And then we put that as a "wrong result" bug fix into the release notes, or so.

Here one of the instances of this bug in live action:

gap> l1:=MakeImmutable(ListWithIdenticalEntries(10,true));
[ true, true, true, true, true, true, true, true, true, true ]
gap> l2:=MakeImmutable(ListWithIdenticalEntries(10,false));
[ false, false, false, false, false, false, false, false, false, false ]
gap> UniteBlist(l2, l1);
gap> l2;
[ true, true, true, true, true, true, true, true, true, true ]

@ChrisJefferson
Copy link
Contributor

I agree, ignire the mutablely issue, we will do a general cleanup later.

@ChrisJefferson
Copy link
Contributor

Just for clarity, I'm happy to merge this as soon as @james-d-mitchell tells me he's made any final changes he wants to make (no rush, and no requirement for changes, that's just why I'm not pressing merge now)

@james-d-mitchell
Copy link
Contributor Author

I believe I've addressed all of the issues (except the mutability one), please let me know if there is anything else.

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.

Looks great to me now, thank you! Once tests have completed, we can merge this. I'll then update my PR, which also adds the mutability checks.

@wilfwilson wilfwilson merged commit 27c845b into gap-system:master Apr 9, 2019
@james-d-mitchell
Copy link
Contributor Author

Thanks @wilfwilson, @fingolfin and @ChrisJefferson for the comments, and apologies for the confusion!

@DominikBernhardt DominikBernhardt 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 Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
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: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants