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

[WIP] Implement dict to receive Object as key #119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HyeockJinKim
Copy link
Contributor

Implement dict struct that takes Object as key
store key and value in array together so that dict is ordered

Issue #118

Implement dict struct that takes Object as key
store key and value in array together so that dict is ordered

Issue go-python#118
@codecov-io
Copy link

Codecov Report

Merging #119 into master will decrease coverage by 0.65%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #119      +/-   ##
=========================================
- Coverage   72.85%   72.2%   -0.66%     
=========================================
  Files          60      60              
  Lines       11949   12057     +108     
=========================================
  Hits         8706    8706              
- Misses       2709    2815     +106     
- Partials      534     536       +2
Impacted Files Coverage Δ
py/dict.go 36.65% <0%> (-35.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c23b14...9e2f6ab. Read the comment docs.

@HyeockJinKim
Copy link
Contributor Author

Since Hash is not implemented properly in gpython, I'm going to implement a dict that takes an Object as a key first.

So, even if an object representing the same value comes in as key, the value is not found correctly.

This can break the test code, so instead of getting rid of StringDict, I will fully implement Dict and replace StringDict with Dict.

Copy link
Collaborator

@ncw ncw left a comment

Choose a reason for hiding this comment

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

I put some notes inline...

Making a Dict is quite a dififcult thing to do (probably why I didn't do it originally!)

Perhaps the thing to do to start with would be to make one with a simple internal structure which is just

type keyValue {
    key Object
    value Object
}

type Dict struct {
    kvs []keyValue
}

And make that work. Yes it won't be very efficient if there are lots of keys, but it should enable us to get the interface right, then we can work on optimizing it.

I note that in go1.14 the go team will expose the internal hashing function which would make implementing this much easier.

}

// String to object dictionary
//
// Used for variables etc where the keys can only be strings
type StringDict map[string]Object

type Dict struct {
keys map[Object]int
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is the right representation alas...

Putting the Object as the key means that you are relying on how Go compares interfaces. It does this with a shallow compare. An interface is basically

  • pointer to type
  • pointer to data

so comparing two interfaces just compares the pointer to type and pointer to data - it doesn't compare the data.

This means that you could insert two objects eg py.String("hello") and py.String("hello") and these would both get inserted into the dictionary.

I think what we want as the key is

  • python type
  • hash of object

And the values need to be a []Object (because different objects can have the same hash).

Copy link
Contributor Author

@HyeockJinKim HyeockJinKim Oct 23, 2019

Choose a reason for hiding this comment

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

There is no hash implementation, so I've got Object as key now.

We only receive hash values as key values. so I'm going to get int as the key.
What do you think about this way?

type Dict struct {
	keys  map[int]int  // int key

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think maybe we should try the really simple version first, then we can make it more efficient later

type dictEntry struct {
    key Object
    value Object
}
type Dict struct {
    entries []dictEntry 
}

Copy link
Member

Choose a reason for hiding this comment

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

@ncw, to be nit-picky, the way py.String is currently implemented, I believe this would work.
see:
https://play.golang.org/p/bDvOEmkim6r

(ie: py.String just contains values and no pointers.)

but ok with going with the KISS version first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed! The problem comes when the py.Object interface is satisfied with a pointer: https://play.golang.org/p/vf4dGmh0Gb4

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.

4 participants