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 a noCopy struct so go vet complains about copying widgets #5520

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

dweymouth
Copy link
Contributor

@dweymouth dweymouth commented Feb 8, 2025

Description:

Pre-2.6 we had an actual lock in BaseWidget so we got this warning "for free". Adding back a dummy sync.Locker implementation so we can still warn if widget structs are copied or passed by value.

image

For #4654

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@coveralls
Copy link

coveralls commented Feb 8, 2025

Coverage Status

coverage: 62.56% (-0.05%) from 62.606%
when pulling cd60f64 on dweymouth:widget-nocopy
into 9677021 on fyne-io:develop.

@dweymouth dweymouth mentioned this pull request Feb 8, 2025
16 tasks
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Thanks for this, but the utility type should be at the end of the file (or at least after the public types)

@Jacalz
Copy link
Member

Jacalz commented Feb 9, 2025

I think it is common practice to have it at the top actually. IIRC, there might be cases where the compile makes the type bigger by adding padding when having a zero size type at the end of the struct.

Jacalz
Jacalz previously approved these changes Feb 9, 2025
@Jacalz
Copy link
Member

Jacalz commented Feb 9, 2025

Oh, sorry. I read that comment incorrectly. I thought you said bottom of the struct. My bad :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants