Implement instVarAt:[put:]

[ltratt]

These are two relatively simple primitives, although while doing this, I realised that some existing functions should be explicitly marked as unsafe (54422e8).

[smarr]

Hm, not the most ideal error message.

[ltratt]

Conceptually I think instance values are an array, so this seems reasonable enough to me. [I half expected them to be exposed as an Array as methods are!]

[vext01]

Wouldn't something like "instVarAt: index 2 out of range" be more user-friendly?

[ltratt]

Yes, but not worth the effort.

[vext01]

OK, I'll leave that one up to you.

[smarr]

Another test could be with a superclass that has fields.

[vext01]

I realised that some existing functions should be explicitly marked as unsafe

Can you expand upon this? The fact that it compiled without the unsafe keyword suggests that -- at-least from a Rust perspective -- the functions are safe. I assume that you are using unsafe as a cautionary note to the programmer then?

[vext01]

Ideas for more tests:

• Getting a larger valid index (e.g. index 5 out of 7).
• Getting a negative index should fail(?) with appropriate error message.
• Zero index should fail(?) with appropriate error message.

[ltratt]

You're right... but it's just too much not-very-useful effort to do this for every indexing operation in SOM.

[vext01]

It's up to you. I tend to test the edge cases, as I don't trust my ability to get it right first time ;)

[ltratt]

The fact that it compiled without the unsafe keyword suggests that -- at-least from a Rust perspective -- the functions are safe.

That's not how safety in Rust works. You can package up arbitrary amounts of deadly unsafe code and if you don't mark the function as unsafe, callers will never know. unsafe on a function is really just a reminder to code readers that they need to be extra careful when calling it.

[vext01]

My curiosity gets the better of me again.

This is unusual. So this is some kind of proxy? Is it transparent? Can I use arithmetic on it?

[ltratt]

That's just normal SOM, it's nothing really to do with this PR.

[vext01]

I gathered it's language semantics, was just curious.

[ltratt]

@smarr's idea for a superclass test is a good one. I'll add that.

[vext01]

That's not how safety in Rust works. You can package up arbitrary amounts of deadly unsafe code and if you don't mark the function as unsafe, callers will never know.

This doesn't match my understanding. If you include anything Rust considers unsafe in a function, Rust will force you to mark the expression or the containing function unsafe.

So if it compiled without unsafe before, then your annotation can't be a requirement, but rather a hint to the programmer.

You could mark any function unsafe on the virtue that somewhere inside is an unsafe block, but by the same token you'd mark main unsafe.

[ltratt]

If you include anything Rust considers unsafe in a function, Rust will force you to mark the expression or the containing function unsafe.

If this was true, everything in Rust would need to be unsafe. unsafe blocks are the way you avoid this.

[vext01]

I think we are agreeing in that aspect, but that's not what I'm questioning.

Rust will force you to mark the expression [with an unsafe block].

As I understand it, you've added a redundant unsafe keyword as a programmer hint. Am I right?

I'm not saying that's wrong to do so, I've just never seen it done and I'm considering if there's utility in such an idiom.

[ltratt]

Test added in 89bc1de.

[vext01]

The unsafe argument was resolved in a (friendly) external discussion.

I think I buy the reasoning :P

[vext01]

Unless @smarr has any further comments, I think this is OK to go in.

[ltratt]

Reflecting SOM-st/SOM#44, I've expanded the test in cf10c7a (it works the same on Java SOM and yksom).

[vext01]

Please squash.

[ltratt]

Squashed.

[vext01]

bors r+

[bors]

Build succeeded:

• buildbot/buildbot-build-script