Strs should be able to handle non-null-terminated strings

[PieterD]

Strs will panic when a string without a null terminator is found. However, it should be easy to handle this case, and add a null terminator if one is not present. This saves having to muck with the strings passed in before, and doesn't break compatibility.

I'll happily do this, if there's an interest in this.

[PieterD]

Well, I did it locally, so I thought I might as well push it.

[dmitshur]

We were considering doing this during API design (/cc @slimsag), and decided to not do it. The motivation was to keep APIs between Str and Strs similar.

It's definitely possible and easy to have Strs not require newline, but the same can't be done for Str without changing its performance characteristics.

I didn't object to this partially because I don't use this low-level gl bindings often, I use the higher level, more idiomatic Go bindings at goxjs/gl and it never requires Go strings to be terminated with a null byte.

But I also agree it's helpful for Str and Strs to share a similar API, given how similarly named they are.

What do you think?

[PieterD]

Thanks for the quick response! And I get where you're coming from.

However, I'm not sure I agree. Str is a conversion, Strs is an allocation. They are already very different, which should be quite clear since Strs returns its own free function. So now you're maintaining an easily remediable error case just because it's named similarly to another function.

But backwards compatibility is important, of course.

So perhaps, to properly address this, there should be ConvertString (which is the current Str), AllocateStrings (the newly fixed Strs), and AllocateString (An allocating, nul-terminator fixing Str returning a free). Str and Strs could then be kept as-is for backwards compatibility. Perhaps even ConvertByte, AllocateByte and AllocateBytes. Anything to reduce memory pressure :)

Of course, 'Convert' and 'Allocate' might be the wrong prefixes.

I hope I'm making sense.

[dmitshur]

What you're saying makes sense.

However, I'd suggest we be thoughtful and not hasty about figuring a possible plan forward here, otherwise we risk creating a bloated and unfriendly API. It's harder to remove stuff than to add.

@slimsag, do you have thoughts on this issue?

[PieterD]

Of course. No worries. There are many ways to go, none of which may be the best one :)

By the way, why didn't you place the conversion functions to a seperate library? When I ran into the GC with gl.ShaderSource, it took me a while to find Str and Strs. It's pretty well hidden.

Perhaps that's an alternative that's more pleasing. In a dedicated library, there'd be more room for the various functions. It would also better accomodate testing.

Just a thought. Thanks for taking the time to review this. And thank you for go-gl too. It saves so much time and headache on header hassles and portability issues :)

[pwaller]

Is this still a problem? What action needs to be taken?

[dmitshur]

Is this still a problem?

I don't think it's a problem, it's a proposal for an enhancement to the API for us to consider.

This was a design decision I've discussed with @slimsag in great detail. I was originally on the side of making Strs work as suggested here, but Stephen at the time made convincing arguments to the contrary and I agreed with him.

What action needs to be taken?

I'd really like to hear @slimsag's thoughts on this since he's familiar with the issue, unless he's not available to look at this for many weeks, then we can try to move forward on our own.

[emidoots]

IMHO we should keep the APIs between Str and Strs identical. Str cannot handle the null terminator because it is a conversion, and not an allocation. Changing this makes the API friendlier, but also worsens performance.

This library should not be concerned about convenience nor safety, that can and should be done at a higher level library. If we had the choice today, I would say we should not ever have had Str or Strs (they should be in a separate external library), but we had Str and with Go 1.6 CGO changes it broke and for API compatibility we had to provide a proper fix.

I'm open to adding these enhancements to a separate library, perhaps github.com/go-gl/glutil or something, but they should not be here IMO. I consider this issue as WontFix.

[dmitshur]

I like the idea of having the Strs helpers in a separate package, but only if there's enough documentation pointing to it so people don't miss them. I consider gl.Strs to be critical to the use of gl packages, it is not an optional utility or something people should be reinventing, unless they have very specific needs.

[mbertschler]

I just saw this line in the documentation (in v2.1):

Strs takes a list of Go strings (with or without null-termination) and returns their C counterpart.

Given the talk here, this comment is wrong?

[dmitshur]

Hmm, I guess I forgot about some finer points of gl.Strs behavior.

The documentation is not wrong about that, see the change in go-gl/glow#71 and the discussion there.

That makes me wonder if the original issue report was using the latest version. gl.Strs itself doesn't panic on non-null-terminated strings anymore.

[dmitshur]

That makes me wonder if the original issue report was using the latest version. gl.Strs itself doesn't panic on non-null-terminated strings anymore.

Ah, this issue/feature request was made before that PR.

This is why it's a good idea to close old issues, they have a higher chance of being less relevant and out of date.

Let's do that now. No one has reported problems with the current behavior since go-gl/glow#71 has been merged, and @pwaller was simply asking about the issue status. Since I forgot about the context/status of the issue, it sent us a on wrong goose-chase.

Closing since this "enhancement" is partially implemented and there's no strong demand for further changes. If there are updates, please make a new issue and feel free to reference this one.