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

 
 

bug #56419: Implementation of newline

Submitter:  Guillaume <gyom>
Submitted:  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
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group Member
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>
Group 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>
Group Member
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>
Group 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>
Group 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>
Group 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>
Group Member
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)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group 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.13-4448.
    Corresponding source code