to panic or not to panic, that is the question

[gaissmai]

New() intercepts a panic in defer() of makeslice and at least for this reason New() is not inlineable and therefore the slice always escapes to the heap. Currently I work around this with From(buffer) but would it also be possible that a MustNew(int) is possibly inlineable if it just propagates the panic.

[lemire]

Sure, pull request invited.

[gaissmai]

maybe you will rethink if you really catch the panic in New() and change the length to 0 under the hood. Perhaps it's generally better to propagate the runtime panics unaltered. You can save some cpu cycles in the hot path and you panic anyway e.g. in extendSet and ShiftLeft .

[lemire]

@gaissmai Yes, although that would have to be a major release and we should carefully document that the constructor may panic. Pull request invited.

[gaissmai]

hmm, I'm not sure whether this requires a major release.

The public interface does not change, only the internal behavior:

• the possibility that New() is now panicking (nobody is happy with a suppressed panic)
• some other panic strings change from self crafted to runtime (the string is no public interface)

I would also leave out the test on panicIfNull, I would rather prefer the runtime panic. The bitset library is used for speed optimization and it is not a high level library, the users of this library prefer to see the runtime panic than a trapped and rewritten panic text.

request for comment

[bbrodriges]

the possibility that New() is now panicking (nobody is happy with a suppressed panic)

This is a big deal. For now there is no way function can panic from user perspective, but changing this behaviour without major version bump can lead to potential unexpected panics in runtime for users.

[gaissmai]

But there are other functions that trigger a panic for the same reason, i.e. if you have specified a tooBig start value for New(tooBig), then New does not panic (and unfortunately also returns no error) but possibly a Set(tooBig) later in the middle of the code.

[gaissmai]

and by the way, the current docstring for New() does not exclude any panic

[lukesandberg]

Just a random note. I wanted to use the Len() function to recover the value i passed to New instead of passing it around separately.

But this issue demonstrated that that was unsafe... not a big deal in my case.. and not a spec issue really since it is documented as a 'hint', but a stronger guarantee here would be nice.

[gaissmai]

I no longer use bitset.New, but take the zero value b := &bitset.BitSet{} and, if I know the maximum length l, followed by b.Set(l).Clear(l)

[lemire]

@lukesandberg

Just a random note. I wanted to use the Len() function to recover the value i passed to New instead of passing it around separately. But this issue demonstrated that that was unsafe... not a big deal in my case.. and not a spec issue really since it is documented as a 'hint', but a stronger guarantee here would be nice.

Fundamentally, the call to New is allocating and can thus panic.

The Len function is trivial:

func (b *BitSet) Len() uint {
	return b.length
}

The New function is effectively just...

	bset = &BitSet{
		length,
		make([]uint64, wordsNeeded(length)),
	}

	return bset

The only difference is that we recover on a panic caused by a lack of memory when the make function is called.

So the only case that matters is the one where you ask for a slice that Go cannot create.

In that case, instead of doing nothing (which would just cause a panic in your code which you would need to handle), we set the length to zero and use a zero slice.

To be clear, in Go, the following is not guaranteed to work...

x := make([]uint64, somelength)
fmt.Println(len(x))

If somelength is an arbitary value, the call to make may fail. This has nothing to do with the library, it is part of the Go language.

You can try it out with this program:

package main

import "fmt"

func main() {
        x := make([]uint64, 9223372036854775807)
        fmt.Println(len(x))
}

It should panic. So the line fmt.Println(len(x)) is never called.

Our API can change but it would change the semantics of the code, so we need to make sure that the users are made aware of the change... otherwise we risk creating unexpected bugs in existing code.

@gaissmai

I no longer use bitset.New, but take the zero value b := &bitset.BitSet{} and, if I know the maximum length l, followed by b.Set(l).Clear(l)

You know this, but for people reading these comments... The code (b.Set(l).Clear(l)) may panic if l is an arbitrary integer. Thus, in general, if you do not want to have a panic, and you cannot control what l is, you should have a defer in your code.

[lukesandberg]

MustNew API would work for me. As does this workaround

[gaissmai]

@lemire btw, it's up to you to close this issue, no problem for me. Thanks a lot for bitset!

[lemire]

@gaissmai Let me be clear: I am not at all dismissing the issue you raised. If people want to close the issue, we will close it...

But, meanwhile, I am inviting a pull request (anyone can do it).

[gaissmai]

will do

[gaissmai]

sorry, closed the pull request, it was a stupid quick shot, another pull request will follow if the quality is nearly as good as the existing code in bitset

[gaissmai]

@lemire the pull request would now be ready on my part, if you have time could you please look over it?

[lemire]

@gaissmai Sure!

[lemire]

Closing.