Re: including a new gnulib module

[Ben Abbott]

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