Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Implement the builtin __dict__ #104

Open
pyjsorg opened this Issue Apr 27, 2012 · 5 comments

Comments

Projects
None yet
1 participant
Contributor

pyjsorg commented Apr 27, 2012

The attached patch implements the dict builtin in classes and instances.
It's a first version and will likely require modifications to keep it all going.
An overview of the patch:

  • The bulk of the patch adds a dict_proxy class based on dict, where most of the work is spent on restricting some elements from the dictionary when the associated object is an instance (as opposed to a class definition), the storage system is changed from what dict uses:

dict.__object.someKey = [someKey, value]

This is replaced by a more direct:

dict_proxy.__object.someKey=value

Which allows for a 1:1 relation with the associated object. And also there's some restrictions on item adding and removal when the associated object is a class definition instead of an instance, following Python's policy on the matter.

  • The code dict_proxy.update and dict_proxy.pop needs further reviewing as it's taken directly from dict, but I'm not sure if the different storage system affects it or not.
  • The comparison code in dict is altered to allow comparison with dict_proxy elements, by reverting the compare and using the dict_proxy object as the compare source.
  • Finally, there's a change in translator.py that was required because when you do:

for x in self.dict:
print x

The code generated doesnt use getattr(self, 'dict') but self['dict'] directly, which might not be set up. It's admitedly hackish, but I couldnt find another way around it, as getattr may or may not be available when the instance and cls_fn code is called in _pyjs.js
This workaround might be needed in some of the other "cases" in that translator block, I haven't experimented much with other use cases.

Original issue: http://code.google.com/p/pyjamas/issues/detail?id=619 (May 24, 2011 17:04:50)

Contributor

pyjsorg commented Apr 27, 2012

From dan.kl...@gmail.com on May 25, 2011 14:54:54:

  1. 33 + if not isinstance(other, dict) and not isinstance(other, dict_proxy):

isinstance works for subclasses, so there should not be any need to check for isinstance(other, dict_proxy)

  1. As dict_proxy is not part of python, and does not implement generic dict proxy, but rather speficially dict proxy (or js object proxy), IMHO it should have name with underscores, as implementation-specific thing.
    •        list_expr = "function(){if ($p['getattr']...
      

Doesn't this call getattr() on each dot access? Under -O mode right now it optimizes getattr to this:

x.z += 1

Translated JS:
runinteractive(function (ctxt) {
...

$module.x['z'] += 1;

}, main);

So you have to check for according compiler flags here

Contributor

pyjsorg commented Apr 27, 2012

From gabo...@gmail.com on May 25, 2011 16:23:26:
About point 3, that's exactly right, I'm making it call getattr because x['dict'] might not be initialized. I'd be happy to hear some alternative to this as I'm not quite sure either...getattr is not there when the _pyjs.js code runs, so it's hard to know where to force an initialization of the dict

Contributor

pyjsorg commented Apr 27, 2012

From dan.kl...@gmail.com on May 25, 2011 16:40:40:
Regarding 3, I mean your change affects other attributes too, besides dict (or I missed check for 'dict' somewhere). getattr() seems reasonable, but only if name of attribute is dict.

Contributor

pyjsorg commented Apr 27, 2012

From gabo...@gmail.com on May 26, 2011 15:52:38:
Patch updated per your comments and added a few fixes on getitem setitem and other places where there was some sort of key hashing.

Contributor

pyjsorg commented Apr 27, 2012

From gabo...@gmail.com on June 15, 2011 14:57:10:
A few more tweaks, it's working pretty well for me, I think this just needs a few tests to go along before it's ready for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment