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 missing imports to core test suite #3781

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

Feoramund
Copy link
Contributor

I forgot about this.

@Feoramund
Copy link
Contributor Author

The core:flags Windows test failed for error code 32.

ERROR_SHARING_VIOLATION

32 (0x20)

The process cannot access the file because it is being used by another process.

It failed on the cleanup part of checking if os.Handle is parsed correctly where it deletes the temporary file. I'm wondering if I should strike that out, since I'm not sure in what way this error is arising, and it's not highly necessary to remove it since it's only a few bytes.

@laytan
Copy link
Collaborator

laytan commented Jun 18, 2024

I am pretty sure that error arises there because you have not closed your file handles before trying to delete the file, the defers run after your attempted deletion.

@Feoramund
Copy link
Contributor Author

Oh, that makes perfect sense. I can just defer the removal too. Why this is allowed on every other platform is beyond me.

@Kelimion Kelimion merged commit dff8a91 into odin-lang:master Jun 18, 2024
6 checks passed
@@ -1113,6 +1113,13 @@ test_if_map_cstrings_get_freed :: proc(t: ^testing.T) {

@(test)
test_os_handle :: proc(t: ^testing.T) {
defer if !testing.failed(t) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be

defer {
    if !testing.failed(t) {
        ...
    }
}

right?

Currently it will check if the test failed at the beginning of the test, which is always false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be a quirk of the syntax, but I did test this before.

package deferred

import "core:fmt"

main :: proc() {
	a := false
	defer if a {
		fmt.println("Hellope.")
	}
	a = true
}

This prints for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a discussion for this since it seems like something that could be confusing. #3782

Copy link
Collaborator

@laytan laytan Jun 18, 2024

Choose a reason for hiding this comment

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

Oh strange I could've sworn it was a thing used to defer something IF the condition is currently true.
Because

if true {
    defer blah()
}

Would execute blah at the end of that if block, not at the end of the wanted scope.

I really thought defer if was created to get around that, @gingerBill ?

I guess I got the wrong impression somewhere and it really is just deferring the if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a quick peek through git log for defer if and all I could find was 118ab60 by @Kelimion.

Copy link
Member

Choose a reason for hiding this comment

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

But mainly I wrote it out that way because just by reading left to right we can rephrase the Odin code into English that highlights we want defer if (deferred conditional), not if defer (conditional defer) if we're unsure which of the two we should write.

That there's the confusion to begin with and that the 2nd phrasing is nonsensical in practice suggests it should be made an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't get if defer a { /* ... */ } to compile, if this was meant as a concrete and not a hypothetical syntax example.

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed. Well, nothing to worry about then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we all meant if x do defer ... here

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I tried if a defer and it doesn't compile.

@Feoramund Feoramund deleted the add-missing-test-imports branch June 30, 2024 03:53
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.

3 participants