octave-maintainers
[Top][All Lists]
Advanced

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

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


From: John W. Eaton
Subject: Re: wavwrite.m - how to commit updated function?
Date: Wed, 5 Sep 2012 17:48:10 -0400

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


reply via email to

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