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 unsafe.Pointer to Nil/NotNil checks #872

Conversation

boyan-soubachov
Copy link
Collaborator

@boyan-soubachov boyan-soubachov commented Jan 24, 2020

@boyan-soubachov
Copy link
Collaborator Author

@georgelesica-wf , having had a look at the Travis build, it seems that there was a bug in Go 1.11 and earlier where IsNil() could not be called on types of unsafe.Pointer. See https://go-review.googlesource.com/c/go/+/155797/

This leaves us with a bit of a quandary.

Do we want to:
a) Remove testify support for anything earlier than Go 1.12?
b) Hold off on this feature until Go 1.11 is sufficiently old enough? What is "sufficiently old enough"?

Thoughts? I'd like to say that I'm leaning towards a but Go 1.12 is still relatively new (just over a year old) and it seems quite unfair to drop support for something that new. Therefore I'm of the opinion that we let this soak for a bit and release it a few minor revisions down the line.

@glesica
Copy link
Collaborator

glesica commented Jan 27, 2020

What about pulling it out into its own file and using a compile flag to only provide it for versions newer than 1.12?

@boyan-soubachov
Copy link
Collaborator Author

What about pulling it out into its own file and using a compile flag to only provide it for versions newer than 1.12?

It's a good idea but it will require us to effectively maintain two separate copies of the Nil/NotNil functions if I am not mistaken. Given that this is an edge-case, I'm not sure it would be worth the added complexity.

Thoughts?

@glesica
Copy link
Collaborator

glesica commented Jan 27, 2020

Yeah, I definitely don't like working with conditionally compiled code, particularly since most IDEs don't make it a great experience in my experience. On the other hand, I definitely feel like 1.12 being a year old is a little too soon to drop support for it, particularly given that Go is starting to show up in "enterprise" settings.

@boyan-soubachov boyan-soubachov added this to the v1.5.0 milestone Jan 30, 2020
@glesica
Copy link
Collaborator

glesica commented Feb 7, 2020

@boyan-soubachov do we want to get this into the next release? I'm thinking we should make the next release 2.0.0 so that would be a nice place to drop support for older versions of Go, I suppose.

@boyan-soubachov
Copy link
Collaborator Author

boyan-soubachov commented Feb 8, 2020 via email

@boyan-soubachov boyan-soubachov self-assigned this Feb 18, 2020
@boyan-soubachov boyan-soubachov modified the milestones: v1.6.0, v2.0.0 Feb 18, 2020
@mvdkleijn mvdkleijn added this to In progress in Roadmap Aug 12, 2020
@dolmen dolmen added assert.IsNil About assert.IsNil/IsNotNil must-rebase labels Jul 31, 2023
@dolmen
Copy link
Collaborator

dolmen commented Nov 8, 2023

Support for unsafe.Pointer has been fixed by #1319.

The issues raised about Go pre-1.11 are not relevant anymore as we do not support those old versions.

Closing.

@dolmen dolmen closed this Nov 8, 2023
Roadmap automation moved this from In progress to Not accepted or Done Nov 8, 2023
@dolmen dolmen removed the must-rebase label Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Roadmap
  
Not accepted or Done
Development

Successfully merging this pull request may close these issues.

Expected nil, but got: (unsafe.Pointer)(nil)
3 participants