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

checker: have stricter checks for .clone() requirement, disallow empty maps #10194

Closed
wants to merge 6 commits into from

Conversation

crthpl
Copy link
Member

@crthpl crthpl commented May 25, 2021

Fixes #10019
Fixes #10011
Depends On #10328

@yuyi98
Copy link
Member

yuyi98 commented May 25, 2021

Personal advices: Array assignments should be Clone by default, If you need direct assignment, you can use unsafe to do that.

@medvednikov
Copy link
Member

Build fails:

/tmp/v/v2.exe.8151136170925915032.tmp.c:18205: error: cannot convert 'extern extern struct array *' to 'struct array'

@crthpl
Copy link
Member Author

crthpl commented May 26, 2021

Personal advices: Array assignments should be Clone by default, If you need direct assignment, you can use unsafe to do that.

@yuyi98 I also support this, but Alex wants clone to be optimized first: #10011 (comment)

@crthpl crthpl marked this pull request as draft May 26, 2021 23:47
@JalonSolov
Copy link
Contributor

I'm confused by the "disallow empty maps" - what does that mean? That you won't be allowed to clone an empty map?

If so... why not?

@crthpl
Copy link
Member Author

crthpl commented May 28, 2021

I'm confused by the "disallow empty maps" - what does that mean? That you won't be allowed to clone an empty map?

If so... why not?

@JalonSolov No, this PR just fixes 2 completely unrelated bugs. The empty map fix is that doing map{} causes an error, and becomes a void value, instead of becoming the 0 type and crashing the compiler.

@JalonSolov
Copy link
Contributor

Gotcha. So disallow completely empty maps, meaning they don't even specify the key and value types.

@crthpl crthpl marked this pull request as ready for review June 3, 2021 23:21
@JalonSolov JalonSolov added the Needs Rebase The code of the PR must be rebased over current master before it can be approved. label Apr 4, 2022
@JalonSolov
Copy link
Contributor

ping?

@JalonSolov JalonSolov closed this Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Rebase The code of the PR must be rebased over current master before it can be approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invalid empty map initialization syntax not detected causing panic Array append sometimes segfaults
4 participants