bugGNU Octave - Bugs: bug #56419, Implementation of newline

 
 

bug #56419: Implementation of newline

Submitted by:  Guillaume <gyom>
Submitted on:  Thu 30 May 2019 11:23:41 AM UTC  
 
Category:  Octave Function Severity:  1 - Wish
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Guillaume Open/Closed:  Closed
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Tue 04 Jun 2019 10:00:56 PM UTC, comment #9: 

Thanks, that makes sense. I wonder if it would be a useful improvement for the test function to take care of this without the need for a semicolon.

Mike Miller <mtmiller>
Project Administrator
Tue 04 Jun 2019 08:36:34 PM UTC, comment #8: 

For %!assert statements the convention is to remove the semicolon because it is unnecessary.  For %!warning and %!error it is occasionally necessary to have a semicolon in order to suppress output.

There are some good examples directly from the newline implementation.

For example, at the command line

[a, b] = newline ()
a =

error: element number 2 undefined in return list

Now make this a BIST test in a file tst_err.m

%!error [a, b] = newline ()

When running the tests I get extraneous output

test tst_err.m
a =

PASSES 1 out of 1 test

Now, add the semicolon

%!error [a, b] = newline ();

and re-run

test tst_err.m
PASSES 1 out of 1 test

It isn't always necessary though.  The newline function, for example, throws an error immediately if there are any arguments to the function.  This happens before any return value is assigned.  So for the BIST test below, no semicolon is required

%!error [a,b] = newline (1)

test tst_err.m
PASSES 1 out of 1 test

At the end of the day, I try not to use semicolons, but occasionally am forced in to it by circumstances.

Rik <rik5>
Project Administrator
Tue 04 Jun 2019 07:10:37 PM UTC, comment #7: 

@Rik I'm curious about the semicolon on %!error tests. I had thought the semicolon was unnecessary and usually remove them when I see them. Can you give an example where it is useful?

Mike Miller <mtmiller>
Project Administrator
Tue 04 Jun 2019 05:06:19 PM UTC, comment #6: 

I checked in your patch here (https://hg.savannah.gnu.org/hgweb/octave/rev/f26b13c80e45).  You can take a look, but I made a few changes to convert it from just a patch to a full changeset.

First, I used your name and e-mail as the author in order to give you credit.  Second, I added a commit message which listed the changes made.
Third, I added the function to the list of new functions for Octave 6.0 in the NEWS file so that people will discover it.  Fourth, I also added the documentation string to the manual in the chapter on Strings.  Fifth, I added a semicolon to the %!error BIST test so that it wouldn't print out unnecessary information when run with 'test strfns.cc'.  Sixth, I added an example in the documentation to show how to use newline() to concated two single lines of text in to a multi-line piece of text.

This is just so you have an idea of what happens behind the scenes.  I know it's a lot to learn, but it does become easier to contribute after a few patches.

Marking as fixed and closing report.

Rik <rik5>
Project Administrator
Tue 04 Jun 2019 08:49:19 AM UTC, comment #5: 

Here is a new version incorporating all of the changes from comment #1.

diff -r 9e5c1d343a2c libinterp/corefcn/strfns.cc
--- a/libinterp/corefcn/strfns.cc       Mon Jun 03 19:03:15 2019 -0700
+++ b/libinterp/corefcn/strfns.cc       Tue Jun 04 09:45:20 2019 +0100
@@ -45,6 +45,31 @@

#include "oct-string.h"

+DEFUN (newline, args, ,
+       doc: /* -*- texinfo -*-
+@deftypefn {} {} newline ()
+Return the character corresponding to a newline.
+
+This is equivalent to @qcode{"@xbackslashchar{}n"}.
+
+@seealso{strcat, strjoin, strsplit}
+@end deftypefn */)
+{
+  if (args.length () != 0)
+    print_usage ();
+
+  static octave_value_list retval = ovl ("\n");
+
+  return retval;
+}
+
+/*
+%!assert (newline (), "\n")
+
+%!error newline (1)
+%!error [a, b] = newline ()
+*/
+
DEFUN (char, args, ,
        doc: /* -*- texinfo -*-
@deftypefn  {} {} char (@var{x})

Guillaume <gyom>
Mon 03 Jun 2019 03:50:28 PM UTC, comment #4: 

@Guillaume.  It is always good to have more people who understand how to write C++ for Octave.  In that case, why don't you update your patch with the suggestions from Mike and then we can commit it.

Rik <rik5>
Project Administrator
Mon 03 Jun 2019 03:18:57 PM UTC, comment #3: 

Thanks, Mike and Rik for the feedbacks. I chose C++ to have this simple function as efficient as possible and, perhaps, selfishly, to learn how to do it. It seems to be built-in in Matlab too.
I'm happy to apply all of the changes of comment #1 and/or switch to an M-file implementation.

Guillaume <gyom>
Thu 30 May 2019 10:33:24 PM UTC, comment #2: 

Is there a reason for this to be done in C++?  Usually we do that when performance is a problem.  But an m-file implementation of this would be quite straightforward and easier to understand for those who only know the Octave language and not C++.

Rik <rik5>
Project Administrator
Thu 30 May 2019 05:16:13 PM UTC, comment #1: 

Thank you for working on this. This is a good idea and a nice and simple built in implementation.

A few comments or suggestions:

  • The doc string should use the Texinfo '@xbackslashchar' macro instead of a literal backslash, and the '@qcode' macro instead of '@code'
  • What is the relevance of the see also references to sprintf and char? Wouldn't functions like strcat, strjoin, or strsplit be more relevant?
  • Is it better to report nargout > 1 with the usage message or with the default error handling? IOW, which of the following error messages is more user friendly?

>> [a, b] = newline ()
error: Invalid call to newline.  Correct usage is:

-- newline ()



>> [a, b] = newline ()
a =

error: element number 2 undefined in return list

IMHO the second is clearer to the user and more consistent with other functions that are called with too many return values requested.

Mike Miller <mtmiller>
Project Administrator
Thu 30 May 2019 11:23:41 AM UTC, original submission:  

Matlab introduced a newline function that returns "\n":

https://www.mathworks.com/help/matlab/ref/newline.html

Here is a possible implementation (with much help from IRC):

diff -r 3d9e72cac668 libinterp/corefcn/strfns.cc
--- a/libinterp/corefcn/strfns.cc       Wed May 29 03:28:05 2019 +0000
+++ b/libinterp/corefcn/strfns.cc       Thu May 30 10:54:46 2019 +0100
@@ -45,6 +45,31 @@

#include "oct-string.h"

+DEFUN (newline, args, nargout,
+       doc: /* -*- texinfo -*-
+@deftypefn {} {} newline ()
+Return the character corresponding to a newline.
+
+This is equivalent to @code{"\n"}.
+
+@seealso{sprintf, char}
+@end deftypefn */)
+{
+  if ((args.length () != 0) || (nargout > 1))
+    print_usage ();
+
+  static octave_value_list retval = ovl ("\n");
+
+  return retval;
+}
+
+/*
+%!assert (newline (), "\n")
+
+%!error newline (1)
+%!error [a, b] = newline ()
+*/
+
DEFUN (char, args, ,
        doc: /* -*- texinfo -*-
@deftypefn  {} {} char (@var{x})

Guillaume <gyom>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by gyom (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-06-04 rik5 StatusPatch Reviewed => Fixed
        Open/ClosedOpen => Closed
    2019-05-30 mtmiller Severity3 - Normal => 1 - Wish
        Item GroupMatlab Compatibility => Feature Request
        StatusNone => Patch Reviewed

    Back to the top


    Powered by Savane 3.5