Re: including a new gnulib module

[c.]

Il giorno 26/lug/2012, alle ore 15.42, John W. Eaton ha scritto:

> On 26-Jul-2012, c. wrote:
> 
> | > Here is a new version of the changeset that adds the new functions in 
> data.cc (and links fine)
> | > I can't push it at the moment as the connection to the mercurial repo at 
> savannah
> | > appears to be down.
> | > 
> | > BTW, is there a better way than copying data as I did in this 
> implementation to create 
> | > an Array<double> from double[] ?
> 
> If you can compute the length of the array separate from allocating
> and filling it, then you could do something like
> 
>  octave_idx_type needed_length = ...;
>  Array<double> buffer (needed_length);
>  function_that_fills_buffer (buffer.fortran_vec ());
> 
> | +extern "C"
> | +{
> | +#include <base64.h>
> | +}
> 
> We should probably ask the gnulib maintainers to add extern "C" to the
> base64.h header file.
> 
> | +      Array<double> in = args(0).array_value ();
> 
> This should probably be const Array<double> since you aren't modifying
> it.
> 
> | +      if (! error_state)
> | +        {      
> | +          char* inc = (char*) in.fortran_vec ();
> 
> If you don't plan to modify the IN vector, then use data () instead of
> fortran_vec.  That way you won't force an unnecessary copy if there is
> more than one reference to the data in the input argument to
> base64_encode.
> 
> In Octave code, we prefer to avoid casts if possible, but if they are
> necessary, then we prefer to use C++-style casts because they are
> easier to find.
> 
> | +          size_t inlen = in.numel () * sizeof (double) / sizeof (char);
> | +          
> | +          char* out;
> | +          
> | +          size_t outlen = base64_encode_alloc (inc, inlen, &out);
> | +          if (out == NULL && outlen == 0 && inlen != 0)
> 
> In C++, it's almost never necessary to use NULL.  0 usually works
> fine.  Or write "!out" instead of "out == 0".
> 
> | +            error ("base64_encode: input array too large.");
> | +          else if (out == NULL)
> | +            error ("base64_encode: memory allocation error.");
> 
> For consistency with other messages in Octave, we don't end error
> messages in periods.
> 
> | +          std::string s (out);
> | +          retval(0) = octave_value (s);
> 
> You should be able to avoid creating the std::string object here.
> There is an
> 
>  octave_value (const char *s, char type = '\'');
> 
> constructor.  The type says whether it is a single- or double-quoted
> string.  I would write
> 
>  retval(0) = octave_value (out);
> 
> to use the default and construct a single-quoted string here.
> 
> Also, there is an octave_value_list constructor that converts a single
> octave_value object to an octave_value_list object with one element,
> so you could write
> 
>  return octave_value (out);
> 
> Finally, the error function simply returns, so lines that follow are
> still executed.  Is that OK to do here, or should you be returning
> early?  For example:
> 
>          if (! out && outlen == 0 && inlen != 0)
>            {
>              error ("base64_encode: input array too large");
>              return retval;
>            }
>          else if (! out)
>            {
>              error ("base64_encode: memory allocation error");
>              return retval;
>            }
> 
> or  
> 
>          if (! out && outlen == 0 && inlen != 0)
>            error ("base64_encode: input array too large");
>          else if (! out)
>            error ("base64_encode: memory allocation error");
> 
>          if (error_state)
>            return retval;
> 
> jwe

I just pushed a changeset that addresses most [*] of the comments above
and adds tests:

http://hg.savannah.gnu.org/hgweb/octave/rev/abc858bc5165

c.


[*] I still need the 'extern "C"' in data.cc until I manage to have that moved 
upstream,
I'll now try to contatct the gnulib people about this