Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

No description provided.

@CyrusNajmabadi
Copy link
Member Author

@RikkiGibson ptal.


Friend Function Intern(s As Char(), start As Integer, length As Integer) As String
Return _stringTable.Add(s, start, length)
Return _stringTable.Add(s.AsSpan(start, length))
Copy link
Contributor

Choose a reason for hiding this comment

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

s.AsSpan(start, length)

VB doesn't support ref structs and I think we should not be using them in VB

Copy link
Member

Choose a reason for hiding this comment

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

We will take #79245 for now to address the concern. Let's work out a solution for the broader set of lexer/TextWindow changes which doesn't require VB to use spans.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really understanding the problem here with this. In what way are ref-structs not supported here? VB seems find creating the span and passing it along.

Copy link
Member Author

Choose a reason for hiding this comment

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

we can still have the overloads that take the char[]+start+length. but i don't see how that is substantively better.

Copy link
Contributor

Choose a reason for hiding this comment

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

In what way are ref-structs not supported here?

VB doesn't support consumption of ref structs. It is supposed to block any attempt to do this, and for many scenarios it does. However, several "holes" remain, that is not intentional though, and it doesn't make ref structs supported in VB.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to know.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) July 3, 2025 18:36
@RikkiGibson RikkiGibson disabled auto-merge July 3, 2025 18:50
Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

It looks like this change takes dependency on VB compiler bug that allows ref structs to be used in some scenarios. I am not comfortable doing this. An alternative approach would be to add special helpers to the shared layer (written in C#) for use from VB, the helpers can use ref structs in their implementation, but should neither take, nor return ref structs.

@CyrusNajmabadi
Copy link
Member Author

sure. i've added back in the helpers to allow vb to call this.

@CyrusNajmabadi
Copy link
Member Author

Closing in favor of #79250

@CyrusNajmabadi CyrusNajmabadi deleted the vbAllocs branch July 3, 2025 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants