Re: wavwrite.m - how to commit updated function?

[Carnë Draug]

On 6 September 2012 08:09, Børge Strand-Bergesen <address@hidden> wrote:
> On Wed, Sep 5, 2012 at 11:48 PM, John W. Eaton <address@hidden> wrote:
>> On  5-Sep-2012, Jordi Gutiérrez Hermoso wrote:
>>
>> | On 5 September 2012 17:10, Børge Strand-Bergesen <address@hidden> wrote:
>> | > What is the procedure for further testing and possible inclusion with
>> | > (the right) Octave (package)?
>> |
>> | Ideally, you'd produce a Mercurial changeset:
>> |
>> |     
>> http://www.gnu.org/software/octave/doc/interpreter/Basics-of-Generating-a-Changeset.html
>> |     http://jordi.inversethought.com/blog/how-to-write-a-patch-for-octave/
>> |
>> | The file you want to patch is scripts/audio/wavwrite.m inside the
>> | Octave repository.
>> |
>> | At a first glance, your fix looks correct. You will need to produce a
>> | commit message for it in this style:
>> |
>> |     http://wiki.octave.org/Commit_message_guidelines
>> |
>> | A few stylistic issues:
>> |
>> |    * You changed the function's name to wavwritex. I suppose this was
>> |      for testing purposes. Please undo this.
>> |
>> |    * You use % and plain ends. This is Matlab-style. In Octave we use
>> |      ## for comments and endfor, endif, endwhile, etc.
>> |
>> |    * In order to distinguish function calls from indexing operations,
>> |      we put a space between the function name and the opening bracket
>> |      in function calls, e.g.
>> |
>> |          a_function_call (x, y, z);
>> |          an_indexing_op(idx);
>> |
>> |    * We don't do one-line ifs. Nor ifs without brackets around the
>> |      condition. Instead of
>> |
>> |          if foo; bar (); endif
>> |
>> |      do
>> |
>> |          if (foo)
>> |            bar ();
>> |          endif
>> |
>> |    * Spaces around + and -. No spaces around * and /, e.g.
>> |
>> |          x + y*z - w/u
>> |
>> |      not
>> |
>> |          x+y*z-w/u
>> |
>> | If you can address these style issues and produce the hg changeset, we
>> | can more easily incorporate your patch into Octave. If it's too
>> | onerous for you to follow Octave coding style and to use hg, you can
>> | just dump your proposed fix in the patch tracker[1], but bear in mind
>> | that *someone* has to do the commit message, style fixes, etc. If you
>> | don't do it, you're just passing that work on to someone else. I may
>> | do it, but since I don't care too much about this function, I may not
>> | get around to it as urgently as you might.
>> |
>> | Thanks for wanting to contribute,
>>
>> And more important than these style issues, it looks like your change
>> uses a for loop and so will call fwrite 3*length(yi) times.  I expect
>> that will be very slow.  Is there some reason that this operation
>> can't be vectorized so that you build one large array using vector
>> operations and then write that to the file in a single call to fwrite?
>> I'm certain that will be much faster than the loop.
>>
>> jwe
>
> Thank you for the feedback. I have edited the file and reattached it.
>
> John, introducing "int24" as an fwrite/fread format would remove the
> need for making a special case out of 24-bit .wav data. It wouldn't
> surprise me if there is a way to call today's fwrite which does this
> in a more efficient way. I just haven't used it enough to tell.
>
> Jordi, I'd appreciate it if you could generate the properly formatted patch.

Hi Børge

you should submit this kind of things to the bug tracker
http://savannah.gnu.org/bugs/?func=additem&group=octave This type of
things get easily lost on a mailing list. And I'm guessing this is
specially true if there's extra work to be done on your patch. Please
open a bug report and attach the file there.

Thanks,
Carnë