SciRuby / nmatrix

Dense and sparse linear algebra library for Ruby via SciRuby
Other
469 stars 133 forks source link

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

Open okkez opened 6 years ago

okkez commented 6 years ago

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?

translunar commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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++.