[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: wavwrite.m - how to commit updated function?
From: |
Carnë Draug |
Subject: |
Re: wavwrite.m - how to commit updated function? |
Date: |
Mon, 8 Oct 2012 02:50:36 +0200 |
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ë
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: wavwrite.m - how to commit updated function?,
Carnë Draug <=