NotificationsYou must be signed in to change notification settings
Fork97
Star948

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

HyeockJinKim wants to merge1 commit intogo-python:main

base:main

Choose a base branch

fromHyeockJinKim:issue118

Open

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

HyeockJinKim wants to merge1 commit intogo-python:mainfromHyeockJinKim:issue118

Conversation

Copy link

Contributor

HyeockJinKim commentedOct 19, 2019

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

Issue#118

Implement dict to receive Object as key

9e2f6ab

Implement dict struct that takes Object as keystore key and value in array together so that dict is orderedIssuego-python#118

Copy link

codecov-io commentedOct 19, 2019•
edited by codecovbot
Loading

Codecov Report

Attention: Patch coverage is0% with108 lines in your changes missing coverage. Please review.

Project coverage is 72.20%. Comparing base(0c23b14) to head(9e2f6ab).
Report is 96 commits behind head on main.

Files with missing lines	Patch %	Lines
py/dict.go	0.00%	106 Missing and 2 partials⚠️

Additional details and impacted files

@@            Coverage Diff             @@##             main     #119      +/-   ##==========================================- Coverage   72.85%   72.20%   -0.66%==========================================  Files          60       60                Lines       11949    12057     +108     ==========================================  Hits         8706     8706- Misses       2709     2815     +106- Partials      534      536       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

Copy link

ContributorAuthor

HyeockJinKim commentedOct 19, 2019

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.

ncw reviewed

Oct 21, 2019

View reviewed changes

Copy link

Collaborator

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

typekeyValue {keyObjectvalueObject}typeDictstruct {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.

py/dict.go

		}

		// 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

ncwOct 21, 2019

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 theObject 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 egpy.String("hello") andpy.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

ContributorAuthor

HyeockJinKimOct 23, 2019•
edited
Loading

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 gotObject as key now.

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

typeDictstruct {keysmap[int]int// int key

Copy link

Collaborator

ncwOct 25, 2019

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

typedictEntrystruct {keyObjectvalueObject}typeDictstruct {entries []dictEntry }

Copy link

Member

sbinetOct 30, 2019

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 waypy.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

ncwOct 30, 2019

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 thepy.Object interface is satisfied with a pointer:https://play.golang.org/p/vf4dGmh0Gb4

Labels

None yet

4 participants

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

HyeockJinKim commentedOct 19, 2019

Uh oh!

codecov-io commentedOct 19, 2019•
edited by codecovbot
Loading

Uh oh!

Codecov Report

Uh oh!

HyeockJinKim commentedOct 19, 2019

Uh oh!

ncw left a comment

Choose a reason for hiding this comment

Uh oh!

ncwOct 21, 2019

Choose a reason for hiding this comment

Uh oh!

HyeockJinKimOct 23, 2019•
edited
Loading

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncwOct 25, 2019

Choose a reason for hiding this comment

Uh oh!

sbinetOct 30, 2019

Choose a reason for hiding this comment

Uh oh!

ncwOct 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Movatterモバイル変換

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

HyeockJinKim commentedOct 19, 2019

Uh oh!

codecov-io commentedOct 19, 2019• edited by codecovbotLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

HyeockJinKim commentedOct 19, 2019

Uh oh!

ncw left a comment

Choose a reason for hiding this comment

Uh oh!

ncwOct 21, 2019

Choose a reason for hiding this comment

Uh oh!

HyeockJinKimOct 23, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncwOct 25, 2019

Choose a reason for hiding this comment

Uh oh!

sbinetOct 30, 2019

Choose a reason for hiding this comment

Uh oh!

ncwOct 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commentedOct 19, 2019•
edited by codecovbot
Loading

HyeockJinKimOct 23, 2019•
edited
Loading