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

Can we stop using ALLOCA_N in C++? #617

Open
okkez opened this Issue Apr 9, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@okkez

okkez commented Apr 9, 2018

Background

I could not compile nmatrix with recent Ruby (2.6.0-preview1 and r63103)
I've tried to compile nmatrix with recent Ruby, but I could not compile it.
See also https://bugs.ruby-lang.org/issues/14668 (Japanese)

shyouhei fixes the issue in r63123 today.

What is the problem

Shyouhei said that we can stop using alloca in C++ code in the comment.

How do you think about this?

@mohawkjohn

This comment has been minimized.

Show comment
Hide comment
@mohawkjohn

mohawkjohn Apr 9, 2018

Member

@okkez Thank you so much for bringing this to our attention.

I read through a translation of the linked page, and wasn't totally clear: what is preferred in place of ALLOCA_N in C++?

Member

mohawkjohn commented Apr 9, 2018

@okkez Thank you so much for bringing this to our attention.

I read through a translation of the linked page, and wasn't totally clear: what is preferred in place of ALLOCA_N in C++?

@okkez

This comment has been minimized.

Show comment
Hide comment
@okkez

okkez Apr 10, 2018

I'm not familiar with C++ specification and nmatrix code, so I could not describe the possibility that we can use C++ functionality more.

It is a general consideration.

VALEU *slice_argv = NM_ALLOCA_N(VALUE, dim);

can replace with

VALUE slice_argv[dim];

This is the simplest case in nmatrix.

For example in other C++ code,

diff --git a/ext/nmatrix/math.cpp b/ext/nmatrix/math.cpp
index 880c1cd..459a3fd 100644
--- a/ext/nmatrix/math.cpp
+++ b/ext/nmatrix/math.cpp
@@ -848,28 +848,29 @@ static VALUE nm_cblas_rot(VALUE self, VALUE n, VALUE x, VALUE incx, VALUE y, VAL
     rb_raise(nm_eDataTypeError, "this operation undefined for integer vectors");
     return Qfalse;
   } else {
-    void *pC, *pS;
-
     // We need to ensure the cosine and sine arguments are the correct dtype -- which may differ from the actual dtype.
     if (dtype == nm::COMPLEX64) {
-      pC = NM_ALLOCA_N(float,1);
-      pS = NM_ALLOCA_N(float,1);
+      float pC[1];
+      float pS[1];
       rubyval_to_cval(c, nm::FLOAT32, pC);
       rubyval_to_cval(s, nm::FLOAT32, pS);
+      ttable[dtype](FIX2INT(n), NM_STORAGE_DENSE(x)->elements, FIX2INT(incx), NM_STORAGE_DENSE(y)->elements, FIX2INT(incy), pC, pS);
     } else if (dtype == nm::COMPLEX128) {
-      pC = NM_ALLOCA_N(double,1);
-      pS = NM_ALLOCA_N(double,1);
+      double pC[1];
+      double pS[1];
       rubyval_to_cval(c, nm::FLOAT64, pC);
       rubyval_to_cval(s, nm::FLOAT64, pS);
+      ttable[dtype](FIX2INT(n), NM_STORAGE_DENSE(x)->elements, FIX2INT(incx), NM_STORAGE_DENSE(y)->elements, FIX2INT(incy), pC, pS);
     } else {
+      void *pC, *pS;
       pC = NM_ALLOCA_N(char, DTYPE_SIZES[dtype]);
       pS = NM_ALLOCA_N(char, DTYPE_SIZES[dtype]);
       rubyval_to_cval(c, dtype, pC);
       rubyval_to_cval(s, dtype, pS);
+      ttable[dtype](FIX2INT(n), NM_STORAGE_DENSE(x)->elements, FIX2INT(incx), NM_STORAGE_DENSE(y)->elements, FIX2INT(incy), pC, pS);
     }
 
 
-    ttable[dtype](FIX2INT(n), NM_STORAGE_DENSE(x)->elements, FIX2INT(incx), NM_STORAGE_DENSE(y)->elements, FIX2INT(incy), pC, pS);
 
     return Qtrue;
   }

okkez commented Apr 10, 2018

I'm not familiar with C++ specification and nmatrix code, so I could not describe the possibility that we can use C++ functionality more.

It is a general consideration.

VALEU *slice_argv = NM_ALLOCA_N(VALUE, dim);

can replace with

VALUE slice_argv[dim];

This is the simplest case in nmatrix.

For example in other C++ code,

diff --git a/ext/nmatrix/math.cpp b/ext/nmatrix/math.cpp
index 880c1cd..459a3fd 100644
--- a/ext/nmatrix/math.cpp
+++ b/ext/nmatrix/math.cpp
@@ -848,28 +848,29 @@ static VALUE nm_cblas_rot(VALUE self, VALUE n, VALUE x, VALUE incx, VALUE y, VAL
     rb_raise(nm_eDataTypeError, "this operation undefined for integer vectors");
     return Qfalse;
   } else {
-    void *pC, *pS;
-
     // We need to ensure the cosine and sine arguments are the correct dtype -- which may differ from the actual dtype.
     if (dtype == nm::COMPLEX64) {
-      pC = NM_ALLOCA_N(float,1);
-      pS = NM_ALLOCA_N(float,1);
+      float pC[1];
+      float pS[1];
       rubyval_to_cval(c, nm::FLOAT32, pC);
       rubyval_to_cval(s, nm::FLOAT32, pS);
+      ttable[dtype](FIX2INT(n), NM_STORAGE_DENSE(x)->elements, FIX2INT(incx), NM_STORAGE_DENSE(y)->elements, FIX2INT(incy), pC, pS);
     } else if (dtype == nm::COMPLEX128) {
-      pC = NM_ALLOCA_N(double,1);
-      pS = NM_ALLOCA_N(double,1);
+      double pC[1];
+      double pS[1];
       rubyval_to_cval(c, nm::FLOAT64, pC);
       rubyval_to_cval(s, nm::FLOAT64, pS);
+      ttable[dtype](FIX2INT(n), NM_STORAGE_DENSE(x)->elements, FIX2INT(incx), NM_STORAGE_DENSE(y)->elements, FIX2INT(incy), pC, pS);
     } else {
+      void *pC, *pS;
       pC = NM_ALLOCA_N(char, DTYPE_SIZES[dtype]);
       pS = NM_ALLOCA_N(char, DTYPE_SIZES[dtype]);
       rubyval_to_cval(c, dtype, pC);
       rubyval_to_cval(s, dtype, pS);
+      ttable[dtype](FIX2INT(n), NM_STORAGE_DENSE(x)->elements, FIX2INT(incx), NM_STORAGE_DENSE(y)->elements, FIX2INT(incy), pC, pS);
     }
 
 
-    ttable[dtype](FIX2INT(n), NM_STORAGE_DENSE(x)->elements, FIX2INT(incx), NM_STORAGE_DENSE(y)->elements, FIX2INT(incy), pC, pS);
 
     return Qtrue;
   }
@wlevine

This comment has been minimized.

Show comment
Hide comment
@wlevine

wlevine Apr 10, 2018

From the linked page:

As an aside, in C ++, there is no necessity to use alloca. Usually VALUE slice_argv[dim]; should be okay. C is also so after C99. I am aware that Ruby still uses alloca because it sticks to the old C.

I think this not totally correct, variable length arrays are part of C99, but not part of any C++ spec, although they are supported by g++.

wlevine commented Apr 10, 2018

From the linked page:

As an aside, in C ++, there is no necessity to use alloca. Usually VALUE slice_argv[dim]; should be okay. C is also so after C99. I am aware that Ruby still uses alloca because it sticks to the old C.

I think this not totally correct, variable length arrays are part of C99, but not part of any C++ spec, although they are supported by g++.

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