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

[ENH] Avoid need for C++ default constructors #4160

Closed
da-woods opened this issue May 14, 2021 · 2 comments
Closed

[ENH] Avoid need for C++ default constructors #4160

da-woods opened this issue May 14, 2021 · 2 comments
Milestone

Comments

@da-woods
Copy link
Contributor

da-woods commented May 14, 2021

Is your feature request related to a problem? Please describe.

Currently stack allocated C++ classes and temps require default constructors since they're initialized at the start of the function.

cdef extern from *:
    cdef cppclass Someclass:
        pass
    SomeClass func() except +

def f(make_someclass: bool):
   if make_someclass:
      a = func()

this generates code along the lines of:

PyObject* f(...) {
   Someclass a
   Someclass __pyx_t_1;
   if (make_someclass) {
      try {
          __pyx_t_1 = make_someclass();
      } catch (...) {
           // exception handling here
      }
      a = __PYX_MOVE_IF_POSSIBLE(__pyx_t_1);
   }
}

This puts restrictions on what c++ types can be used on the stack (or even as return types of functions, because of the temps creates)

Describe the solution you'd like

std::optional provides a way around this. It'd be possible to create the stack variables as:

std::optional<Someclass> a;
std::optional<Someclass> __pyx_t_1;

These keep keep track of whether it's been assigned, and handle destruction etc. as needed.

My proposal is that this should be an opt-in feature, controlled by a directive. There's a number of good reasons not to do it by default:

  • C++17 only (which is a fairly hefty requirement)
  • Potential performance changes (due to extra space usage, and "initialized tests"
  • Changes in Cython behaviour - accessing an uninitialized C++ variable becomes a potential crash rather than an access to a valid default-initialized object.

I think the Cython type system would have to know about the std::optional and treat them as a slightly different type to the raw classes - *optional gets to the underlying class, which is a trivial coercion, but one that can't happen automatically.

Describe alternatives you've considered

The status quo.

Reimplement std::optional (i.e. it's a library feature, I don't think it requires a significant compiler support). That would reduce the compiler requirement, but require more effort. It could also be implemented in terms of std::optional first then revisited if need.

Additional context

Exceptions from constructors/operators are a potential problem. But equally exceptions from default/copy/move constructors/operators are an issue in Cython currently.

@da-woods da-woods added this to the wishlist milestone May 14, 2021
@maxbachmann
Copy link
Contributor

C++17 only (which is a fairly hefty requirement)

There are backports even for C++98

Potential performance changes (due to extra space usage, and "initialized tests"

As an alternative we could use std::aligned_storage and placement new as of C++11: https://en.cppreference.com/w/cpp/types/aligned_storage

@da-woods
Copy link
Contributor Author

da-woods commented May 14, 2021

C++17 only (which is a fairly hefty requirement)

There are backports even for C++98

True. Notably the Boost version which I suspect is more of a forward port.

I was mainly trying not to write it myself (I expect there's a lot of details to get right to do it well...) and partly thinking that some standard library dependency is probably acceptable for the initial implementation of an optional feature. It can always been made to work more widely later.

Potential performance changes (due to extra space usage, and "initialized tests"

As an alternative we could use std::aligned_storage and placement new as of C++11: https://en.cppreference.com/w/cpp/types/aligned_storage

I don't think aligned_storage plus placement new would help too much on it's own - you'd still need to track which objects are initialized so that they can be destructed and once you've done that you've largely just reimplemented optional.

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

No branches or pull requests

3 participants