Skip to content

Fix if expressions in lock expression#14384

Merged
spytheman merged 3 commits intovlang:masterfrom
crthpl:ifexpr-in-lock
May 15, 2022
Merged

Fix if expressions in lock expression#14384
spytheman merged 3 commits intovlang:masterfrom
crthpl:ifexpr-in-lock

Conversation

@crthpl
Copy link
Copy Markdown
Member

@crthpl crthpl commented May 12, 2022

No description provided.

Comment thread vlib/sync/array_rlock_test.v Outdated
@@ -1,5 +1,5 @@
fn test_shared_modification() {
shared foo := &[2, 0, 5]
shared foo := [2, 0, 5]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the removals of & seem to be unrelated to the PR?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

shared implies a reference and the underlying type looks like this;

struct __shared__Array_int {
	sync__RwMutex mtx;
	Array_int val;
};

and then whenever you creacte a shared foo, you create a reference to __shared__Array_int, not the Array_int:

__shared__Array_int* foo = (__shared__Array_int*)__dup_shared_array(&(__shared__Array_int){.mtx = {0}, .val =
new_array_from_c_array(3, 3, sizeof(int), _MOV((int[3]){2, 0, 5}))}, sizeof(__shared__Array_int));

This was generated with current V, and it didn't generate a reference to the array anyways.
This PR makes assignments always use expr_with_cast, which doesn't allow casting a reference to shared:

g.error('cannot convert reference to `shared`', expr.pos())

You need a reference to __shared__Array_int because the mutexes all need to be the same one in memory, and having a shared reference is usually (always?) never what you want.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This cgen error is not triggered by any of the existing tests, and the tests that are changed, compile fine on both master and the branch of this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The file vlib/v/tests/shared_if_expr_test.v is the only test file that should stay in the PR imho.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For example the change in vlib/sync/array_rlock_test.v is not related to the PR:
image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@spytheman he's removing unnecessary &[] from the code. It's already handled by V when it sees it's shared.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand that.
But it is handled by V now.
It is not guaranteed to be handled by V in the future.
By not changing these tests, it would be, because they will break and the CI will tell us when that happens.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

btw, I have no objections to making it an explicit checker error, and a corresponding test for it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see, ok I agree.

This reverts commit 368df24.
@spytheman spytheman merged commit e4065bd into vlang:master May 15, 2022
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