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

Add unbounded pointer depth in pure Python 3.7+ #2571

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kne42
Copy link

@kne42 kne42 commented Aug 22, 2018

Leverages module-level __getattr__ as introduced in PEP 562 (Python 3.7+) to enable unbounded depth on pointers in interpreted Python based on their names; i.e. from cython import ppppppp_int will now be valid.

@@ -461,3 +461,16 @@ def threadid(self):
import sys
sys.modules['cython.parallel'] = CythonDotParallel()
del sys


def __getattr__(name, module=__name__):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice idea! Worth a docstring, though, to explain what this is used for and why it's done this way.

depth = len(match.group(1))
type = match.group(2)

if type in int_types + float_types + complex_types + other_types:
Copy link
Contributor

Choose a reason for hiding this comment

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

Might it be worth caching this list, or checking each individually? It'll only be used during import yes, but it's better to not be more expensive than necessary.

Copy link
Contributor

@scoder scoder Aug 25, 2018

Choose a reason for hiding this comment

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

Agreed. Can be done by assigning the whole concatenation to an additional default argument, for example.
BTW, type overrides the builtin name, so I'd rather call the variable type_name.

@scoder
Copy link
Contributor

scoder commented Aug 25, 2018

BTW, the fact that travis is unhappy is unrelated to this PR. Would be nice if you could rebase it on latest master.

@scoder
Copy link
Contributor

scoder commented Aug 31, 2018

Any update on this?

@kne42
Copy link
Author

kne42 commented Aug 31, 2018 via email

@scoder
Copy link
Contributor

scoder commented Aug 31, 2018

All fine, no rush, just wanted to check back.
Yep, runnable tests go into tests/run/. Specifically, the pure* tests would be relevant examples here since they are executed in both Cython and Python. You can exclude older Python versions from running the pure Python tests by adding a pure3.7 tag at the top, which also suggests that this would best be a test on its own, just name it pure_py37.py then.

@@ -461,3 +461,23 @@ def threadid(self):
import sys
sys.modules['cython.parallel'] = CythonDotParallel()
del sys

import functools
@functools.lru_cache()
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, the fact that the feature is only available in Py3.7+ does not mean that the code will only be used there. This file should stay compatible with Py2.6+ for now, which doesn't have functools.lru_cache.

import cython


def test_module_getattr_pointers:
Copy link
Contributor

Choose a reason for hiding this comment

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

Syntax error.



def test_module_getattr_pointers:
cython.p_int
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be used in a valid context, e.g. use int_ptr: cython.p_int as a variable declaration. Preferably, also find a way to make actual use of these variables. :)

@HSR05
Copy link

HSR05 commented Mar 19, 2019

@scoder I'll try looking into it. Can you give me some guidance on the work thats already done and what needs to be done. I would be really thankful. 👍

@TeamSpen210
Copy link
Contributor

Instead of using lru_cache, you could just add the constructed pointers to the global namespace. That way future lookups will grab the new attribute and bypass __getattr__ entirely.

@scoder
Copy link
Contributor

scoder commented Mar 24, 2019

@TeamSpen210, sounds good.
@HSR05, the test needs fixing and the implementation should work with Py2.7.

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

Successfully merging this pull request may close these issues.

None yet

4 participants