Add support for globals. #90

ummarikar · 2020-01-15T02:24:10Z

Add a hashmap which maps symbols to their corresponding object and retrieve an object from this hashmap when its symbol is referenced during runtime.

ummarikar · 2020-01-15T02:26:10Z

This PR is unfinished and is only "fully" implemented for the Integer class, just wanted to submit it so we can discuss my implementation before I finish the rest and subsequently squash. FYI the test passes.

src/lib/vm/core.rs

ltratt · 2020-01-15T15:31:31Z

yksom currently doesn't handle globals in an ideal way in the sense that they can't be reassigned to (which, I think, SOM allows). Before we go down the full class primitives route, we should probably get the semantics of this right because we may then be able to merge / remove the Builtin opcode which currently does something very similar to what you've called Globals.

ummarikar · 2020-01-15T16:25:37Z

How do you think we should do it?

ltratt · 2020-01-15T16:26:06Z

I think you should make some test files that show what happens in other SOMs when you assign to a global, and then see how such behaviour might best be implemented in yksom.

ummarikar · 2020-01-15T16:29:53Z

By assigning globals do you mean what was discussed here? #82 (comment)

ltratt · 2020-01-15T16:30:55Z

Yes, but there might be other ways of doing the same thing too (e.g. assigning to the variable Integer or true might work for all I know).

ummarikar · 2020-01-15T16:31:54Z

Great I'll start that now and will let you know how I get on.

smarr · 2020-01-15T16:39:49Z

@ummarikar please try all the things you can come up with, especially the things that should not work... I am, well, not entirely sure which cases are handled correctly...

Via system one can definitely change the globals. Using a simple assignment, I would hope not, but oh well...

ummarikar · 2020-01-15T18:24:45Z

@ummarikar please try all the things you can come up with, especially the things that should not work... I am, well, not entirely sure which cases are handled correctly...

Via system one can definitely change the globals. Using a simple assignment, I would hope not, but oh well...

Will do! Thanks

ummarikar · 2020-01-17T01:07:22Z

Working on global:put: as I am typing this, just wanted to submit what I have done thus far so it can be critiqued.

ltratt · 2020-01-18T17:03:20Z

I think we really need to see "hard" examples (as @smarr suggested) before critiquing the code, as the precise SOM semantics could change what we perceive to be the best route.

ummarikar · 2020-01-18T17:19:05Z

Yes apologies, I’ve been working on something substantial but have hit a couple of roadblocks along the way. Will submit something either today or tomorrow.

ummarikar · 2020-01-19T23:55:32Z

Still struggling to get this right don’t want to ask for help yet though so I will continue to try

lang_tests/system3.som

src/lib/vm/core.rs

ummarikar · 2020-01-25T00:35:48Z

@smarr what are the intended semantics for (System global: #Integer put: 42) println.? Does global:put: return anything such as the value you give it (42 in this case)?

ummarikar · 2020-01-25T01:00:23Z

Ignore me I had a look at the code for global:put: in som-java and referred to the comment you left here - #82 (comment) and found that it returns nothing.

smarr · 2020-01-25T09:36:23Z

@ummarikar hm, I think this is a good question.
I believe it is common practice to return either self or the value from such mutating methods.

See for instance: (Array new: 6) at: 4 put: 66, which returns 66.

I would assume that returning the value is the most useful thing to do here,
given the #at:put: precedence, and that the value may be derived from an expression.
Returning system or nil seems not ideal.

ummarikar · 2020-01-25T10:19:30Z

@smarr I agree! I’ll have global:put: return value.

src/lib/vm/core.rs

ummarikar · 2020-01-28T20:13:22Z

This is my global: and global:put: solution. I have changed the name of the tests too so they are more descriptive.

ummarikar · 2020-01-28T20:38:27Z

Just added the classes of som to ast_to_instrs.

ltratt · 2020-01-28T20:52:24Z

Before I look into this in detail, can I check that you've run all the new tests on another SOM (e.g. Java SOM) to check that they behave exactly the same?

ummarikar · 2020-01-28T20:58:45Z

I tried this in SOM but I got an error that global: and global:put: is not in the System class. Will try it in another SOM soon.

lang_tests/system_global.som

lang_tests/system_global_invalid_symbol_err.som

lang_tests/system_global_put_invalid_symbol_err.som

lang_tests/system_global_put_type_err.som

ummarikar · 2020-01-28T21:25:26Z

Okay now that @smarr kindly, pointed out that I have been mistaking System for system I have run tests on java SOM and I believe I understand the semantics now. Please ignore my code above and I will start working on the changes.

smarr · 2020-02-06T09:07:20Z

I am not sure why the boolean is necessary. And I do not understand the logic where the boolean is tested.

In other SOM implementations, this boolean doesn't seem to be necessary. Metaclasses are also just instances of Class.

---> Integer class class superclass println
Class

Again, your PR is growing :) So, perhaps this is fine for the moment, but probably wants to be looked at at a later time again.

ltratt · 2020-02-06T09:31:03Z

I agree with @smarr that we need to get this PR to a resolution. It's partly my fault for asking questions.

@ummarikar I suggest splitting this PR into two. Let's make this PR everything before the "adding metaclass" commit. You can do something like git checkout -b metaclass ; git checkout class_primitivies; get reset --hard 4e538bc719 to do this. Then we can work out what we need to do to fix the pre-metaclass stuff. Maybe the simplest correct thing is to just delete that int_class test that was causing us confusion? Once we've got this PR to a point we're happy with, we can then move on to the actual metaclass PR.

ummarikar · 2020-02-06T12:36:56Z

Done

ltratt · 2020-02-06T15:49:14Z

lang_tests/system_global.som

+    false
+"
+
+system_global = (


To double check: does this test run identically on another SOM?

Everything except the last three lines but per @smarr 's request we kept nil, true and false as non-globals.

Right, got it.

ltratt · 2020-02-06T15:53:43Z

src/lib/vm/core.rs

+
+        let val = self.compile(&path, inst_vars_allowed);
+        let idx = self.add_symbol(name.to_string());
+        let globals = unsafe { &mut *self.globals.get() };


As elsewhere, we can't (generally) safely assign UnsafeCells to a variable, so unless we really need to, let's not do so.

In this case, we can probably change the code below to something like: unsafe { &mut *self.globals.get() }.entry(&idx).and_modify(|e| *e = val) (notice that I think the Inst::new is still bogus, right?).

ltratt · 2020-02-06T15:54:02Z

src/lib/vm/core.rs

@@ -326,6 +343,15 @@ impl VM {
                    unsafe { &mut *self.stack.get() }.push(Double::new(self, i));
                    pc += 1;
                }
+                Instr::Global(symbol_off) => {
+                    debug_assert!(unsafe { &*self.symbols.get() }.len() > symbol_off);
+                    if let Some(global) = unsafe { &mut *self.globals.get() }.get(&symbol_off) {


We can probably do the entry trick here too.

Ah, no, ignore that comment -- sorry!

ltratt · 2020-02-06T15:56:52Z

src/lib/vm/core.rs

+
+                if let Some(i) = unsafe { &mut *self.reverse_symbols.get() }.get(s) {
+                    unsafe { &mut *self.globals.get() }.insert(*i, value.clone());
+                    unsafe { &mut *self.stack.get() }.push(value.clone());


My guess is that the second clone is unnecessary, but I might be wrong.

ummarikar · 2020-02-06T16:43:17Z

Done.

ummarikar · 2020-02-06T16:44:58Z

src/lib/vm/core.rs

+        let idx = self.add_symbol(name.to_string());
+        unsafe { &mut *self.globals.get() }
+            .entry(idx)
+            .or_insert(val.clone());


and_modify did not pass the tests but this does.

ltratt · 2020-02-06T17:39:19Z

src/lib/compiler/ast_to_instrs.rs

+                        match lex_string {
+                            "nil" => vm.instrs_push(Instr::Builtin(Builtin::Nil)),
+                            "false" => vm.instrs_push(Instr::Builtin(Builtin::False)),
+                            "system" => {


Isn't this clause unnecessary in the sense that it's covered by the _ clause below?

Yes, must have forgotten to remove it. Done now

ltratt · 2020-02-07T18:03:34Z

Please squash.

ummarikar · 2020-02-07T18:31:41Z

src/lib/vm/core.rs

@@ -44,6 +44,8 @@ pub enum VMError {
        expected: ObjType,
        got: ObjType,
    },
+    /// Tried to access a global before it being initialised.


The comment that was here before was incorrect so I changed it to this. Everything is squashed now!

ltratt · 2020-02-07T20:24:22Z

Should these 3 commits be squashed down into 1? The last commit seems to fix fairly big problems in the first 1 or 2 commits, so I'm not sure those are very useful to keep in the history. However, I can be convinced otherwise.

ummarikar · 2020-02-07T20:43:37Z

You make a very good point I'll squash this into one.

ummarikar · 2020-02-07T20:44:39Z

Squashed.

ltratt · 2020-02-07T20:47:21Z

Would I be right in thinking that the commit title should really be "add support for globals"?

ummarikar · 2020-02-07T21:37:16Z

Yes you would, one second.

Add a hashmap which maps symbols to their corresponding object and retrieve an object from this hashmap when its symbol is referenced during runtime.

ummarikar · 2020-02-07T21:38:56Z

Done :)

ltratt · 2020-02-07T22:10:45Z

bors r+

90: Add support for globals. r=ltratt a=ummarikar Add a hashmap which maps symbols to their corresponding object and retrieve an object from this hashmap when its symbol is referenced during runtime. Co-authored-by: Umar Marikar <[email protected]>

bors · 2020-02-07T22:14:17Z

Build succeeded

buildbot/buildbot-build-script

ltratt reviewed Jan 15, 2020

View reviewed changes

src/lib/vm/core.rs Outdated Show resolved Hide resolved

ummarikar commented Jan 20, 2020

View reviewed changes

lang_tests/system3.som Outdated Show resolved Hide resolved

ummarikar commented Jan 20, 2020

View reviewed changes

src/lib/vm/core.rs Outdated Show resolved Hide resolved

ummarikar commented Jan 25, 2020

View reviewed changes

src/lib/vm/core.rs Show resolved Hide resolved

smarr mentioned this pull request Jan 25, 2020

System>>#global:put: should return value SOM-st/SOM#25

Open

2 tasks

ummarikar commented Jan 25, 2020

View reviewed changes

src/lib/vm/core.rs Outdated Show resolved Hide resolved

ummarikar commented Jan 28, 2020

View reviewed changes

src/lib/vm/core.rs Outdated Show resolved Hide resolved

smarr reviewed Jan 28, 2020

View reviewed changes

ummarikar force-pushed the class_primitives branch from 3c1c839 to 4e538bc Compare February 6, 2020 12:36

ltratt reviewed Feb 6, 2020

View reviewed changes

ummarikar commented Feb 6, 2020

View reviewed changes

ltratt reviewed Feb 6, 2020

View reviewed changes

ummarikar force-pushed the class_primitives branch 2 times, most recently from f86e586 to 66c8704 Compare February 7, 2020 18:28

ummarikar commented Feb 7, 2020

View reviewed changes

ummarikar force-pushed the class_primitives branch from 66c8704 to db065ec Compare February 7, 2020 20:44

Add support for globals.

3c0ce78

Add a hashmap which maps symbols to their corresponding object and retrieve an object from this hashmap when its symbol is referenced during runtime.

ummarikar force-pushed the class_primitives branch from db065ec to 3c0ce78 Compare February 7, 2020 21:38

ummarikar changed the title ~~Add support for class primitives.~~ Add support for globals. Feb 7, 2020

bors bot merged commit 3c0ce78 into softdevteam:master Feb 7, 2020

ummarikar deleted the class_primitives branch February 12, 2020 16:01

Add support for globals. #90

Add support for globals. #90

Conversation

ummarikar commented Jan 15, 2020 • edited Loading

ummarikar commented Jan 15, 2020

ltratt commented Jan 15, 2020

ummarikar commented Jan 15, 2020

ltratt commented Jan 15, 2020

ummarikar commented Jan 15, 2020

ltratt commented Jan 15, 2020

ummarikar commented Jan 15, 2020

smarr commented Jan 15, 2020

ummarikar commented Jan 15, 2020

ummarikar commented Jan 17, 2020

ltratt commented Jan 18, 2020

ummarikar commented Jan 18, 2020

ummarikar commented Jan 19, 2020

ummarikar commented Jan 25, 2020 • edited Loading

ummarikar commented Jan 25, 2020 • edited Loading

smarr commented Jan 25, 2020

ummarikar commented Jan 25, 2020 • edited Loading

ummarikar commented Jan 28, 2020 • edited Loading

ummarikar commented Jan 28, 2020

ltratt commented Jan 28, 2020

ummarikar commented Jan 28, 2020

ummarikar commented Jan 28, 2020

smarr commented Feb 6, 2020

ltratt commented Feb 6, 2020

ummarikar commented Feb 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ummarikar commented Feb 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltratt commented Feb 7, 2020

Choose a reason for hiding this comment

ltratt commented Feb 7, 2020

ummarikar commented Feb 7, 2020

ummarikar commented Feb 7, 2020

ltratt commented Feb 7, 2020

ummarikar commented Feb 7, 2020

ummarikar commented Feb 7, 2020

ltratt commented Feb 7, 2020

bors bot commented Feb 7, 2020

Build succeeded

ummarikar commented Jan 15, 2020 •

edited

Loading

ummarikar commented Jan 25, 2020 •

edited

Loading

ummarikar commented Jan 25, 2020 •

edited

Loading

ummarikar commented Jan 25, 2020 •

edited

Loading

ummarikar commented Jan 28, 2020 •

edited

Loading