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

Non-type template parameters #426

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

Conversation

insertinterestingnamehere
Copy link
Contributor

This is some of my previous work with non-type template parameters. It is very nearly finished. Depending on the final decision regarding which syntax should be used, this PR may be a moot point, but I figured I should post it to make the changes more publicly visible.

Things that have yet to be done here:

  • Allow references to generic c++ objects to be used as template parameters
  • If possible, check that references used are only for global variables
  • Add tests for using references and enum types as template parameters

@scoder
Copy link
Contributor

scoder commented Sep 13, 2015

What is the status of this? Is it still "nearly finished"? Does it need to be improved in order to be useful?

If the syntax needs more discussion, could you bring it up on the devel mailing list again?

The last travis build failed, but that might have been a problem in master at the time. Could you rebase it on current master?

@insertinterestingnamehere
Copy link
Contributor Author

Rebased.

When I originally brought this up on the mailing list there wasn't any discussion for a while, so I opted to implement it as I saw fit. Around the time this was nearly finished, the discussion turned toward using a different syntax. I submitted this PR as a work in progress that could serve as a nice working example.

I didn't have time to implement the other syntax suggested, particularly since the changes here are much less invasive. Implementing the other syntax will introduce more corner cases in template instantiation that will be hard to manage effectively in the Cython code. On the other hand, the version I have here is more likely to give users unusual C++ compilation errors.

The remaining test failures and cleanup should be fairly easy. I can take care of them in the next week or so if needed. I don't have time to implement the other syntax right now though. I'll post on the mailing list some time in the next day or two to see if there is still any interest in these patches.

@scoder
Copy link
Contributor

scoder commented Sep 30, 2015

The tests are failing now. I revived the mailing list thread to move this forward.

@insertinterestingnamehere
Copy link
Contributor Author

Sorry for the delays here. I'm still working on this periodically, though most of the time I've spent thus far has been in tracing what Cython does in various related cases for parsing array-like expressions. What needs to happen to get the alternate syntax working is I need to figure out a way to have the array declarator accept more than just a comma-separated list of names while still making that an error case for things like arrays.

@AcidLeroy
Copy link

👍 for this pull request. It would be super handy to expose types like std::array<class T, size_t N>. Currently, I have to expose arrays using very hack like syntax:

cdef extern from "<array>" namespace "std" nogil:
    cdef cppclass array2 "std::array<double, 2>":
        array2() except+
        double& operator[](size_t)
        ...

it would be great to have this automatically create arrays from np.arrays and lists like vector does.

@jakirkham
Copy link
Contributor

Indeed @AcidLeroy. Was trying to wrap up std::array in PR ( #472 ), but am stuck without this.

@@ -3557,9 +3563,19 @@ def calculate_result_code(self):
else:
assert False, "unexpected base type in indexing: %s" % self.base.type
elif self.base.type.is_cfunction:
template_indices = []
for param in self.type_indices:
if isinstance(param, PyrexTypes.CType):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be wrapped in some kind of tree nodes, rather than sitting right next to nodes in the same mixed-types list.

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