octave-maintainers
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: including a new gnulib module


From: Ben Abbott
Subject: Re: including a new gnulib module
Date: Mon, 30 Jul 2012 19:41:28 -0400

On Jul 30, 2012, at 7:25 PM, Ben Abbott wrote:

> 
> On Jul 29, 2012, at 1:23 PM, c. wrote:
> 
>> 
>> 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
> 
> I'm seeing the error below.
> 
>       data.cc:43:20: fatal error: base64.h: No such file or directory 
> 
> Ben

I did ...

        cd gnulib
        git pull

... and a fresh build beginning with autogen finished.

Ben




reply via email to

[Prev in Thread] Current Thread [Next in Thread]