bugGNU Octave - Bugs: bug #49454, strread commentstyle doesn't work...

 
 

bug #49454: strread commentstyle doesn't work as expected

Submitter:  None
Submitted:  Thu 27 Oct 2016 07:15:04 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  3 - Low Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  philipnienhuis
Originator Name:  Originator Email:  -email is unavailable-
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

Mon 07 Nov 2016 06:26:56 PM UTC, comment #14: 

TTBOMK strread.m hasn't changed since 4.2.0 and 4.3+ were branched off from 4.1.0+ so it should be safe to backport it to stable.
But there is probably no hurry, it could be in a 4.2.1 bug fix release as well.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 02 Nov 2016 08:55:17 PM UTC, comment #13: 

You could if you like.  I think the fact that it took forever for someone to notice this means it isn't a requirement to fix it before the next release.  I guess if you 'hg graft' it from default to stable and there are a bunch of merges to figure out then it can be done.

Rik <rik5>
Group administrator
Wed 02 Nov 2016 06:33:30 PM UTC, comment #12: 

Would it be useful to backport the fix to stable? as it is a mere bug fix.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 29 Oct 2016 08:55:58 AM UTC, comment #11: 

Rik:
In hindsight I think I could have been more clear, sorry.
The word "FIXME" shouldn't have been there in the first place.

Thanks for reviewing & pushing

Philip Nienhuis <philipnienhuis>
Group Member
Fri 28 Oct 2016 10:50:50 PM UTC, comment #10: 

Oh, I see.  I don't think we need to support decade-old operating systems.  I know we dropped support for some of the arcane compilers like Cray, and probably even DEC.  I reverted my cset, although I still removed the FIXME since we aren't going to do it.  See http://hg.savannah.gnu.org/hgweb/octave/rev/90c3839825a3.

Rik <rik5>
Group administrator
Fri 28 Oct 2016 08:51:51 PM UTC, comment #9: 

@Rik:
The FIXME was about old Macintosh OS, not the current OS-X.
Nowadays OS-X also has LF ("\n") as EOL char.

That's why I left the FIXME in there - do we really need to cater for an operating system from over a decade ago?

Philip Nienhuis <philipnienhuis>
Group Member
Fri 28 Oct 2016 05:47:26 PM UTC, comment #8: 

I added to the commit message what the changes in the code were and then checked it in on the development branch (http://hg.savannah.gnu.org/hgweb/octave/rev/644dcdfa0348).

There was a FIXME in the code about handling comments on Mac platforms which I fixed in this cset (http://hg.savannah.gnu.org/hgweb/octave/rev/b100f76b991d).

Closing report.

Rik <rik5>
Group administrator
Fri 28 Oct 2016 01:23:51 PM UTC, comment #7: 

@Mike comment #2:

Matlab r2012a has the same issue as Octave; comments need a delimiter ("word boundary") around them. So yes, Octave's textscan is Matlab-compatible here, but it's a little disappointing.

>> format compact
>> C = textscan (['hello#foo' char(10) 'world'], '%s', 'commentstyle', '#')
C =
    {2x1 cell}
>> C{1}
ans =
    'hello#foo'
    'world'
>> C = textscan (['hello #foo' char(10) 'world'], '%s', 'commentstyle', '#')
C =
    {2x1 cell}
>> C{1}
ans =
    'hello'
    'world'
>>


Octave's previous textscan.m was better in this respect; it could also distinguish cuddling literals.
This was discussed with Lachlan in patch #8783 in a.o., comment #99 and comment #5.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 28 Oct 2016 10:15:42 AM UTC, comment #6: 

Please try the following cset

If you think it works fine it could be backported to stable as well.

(file #38830)

Philip Nienhuis <philipnienhuis>
Group Member
Fri 28 Oct 2016 09:16:12 AM UTC, comment #5: 

(My work appointment (on my regular off-day) was just canceled, so I got some time :-) )

The issue is twofold:

  • as outlined below, strread.m needs the "endofline" value to assess comment endings.
  • strread.m as it stands always wants to eat the comment ending character (default "endofline" value), but in "open" comment styles like "shell" that is non-existent. Consequently in the example in the comments below it eats the newline following the comment but that is actually a delimiter;


As "open" comment styles generally end with a newline (what else?) I think my suggestion in my previous comment would work fine. I already have a fix but need to add a few tests.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 27 Oct 2016 08:44:46 PM UTC, comment #4: 

Yeah it's quite a while since I fiddled with strread.m / textread.m / textscan.m:-)

Some observations:
------------------
The plan was (is?) to make strread.m a front end for textscan(); it used to be the other way round until ~a year ago.

strread.m in Matlab is not deprecated but it is recommended to rather use textscan. (In the Matlab docs the stanza that strread "will be removed in a future version of Matlab" has disappeared; so I conclude that deprecation sensu stricto is not planned.)

I made a start with converting strread.m a year ago after Lachlan created the binary txtscan() but ran in several issues. As textscan is a better alternative, also recommended by Matlab, I wonder how much energy we should put into keeping strread.m up-to-date anyway.
AFAIK TMW doesn't do much about it either. For example, about a year ago I entered a bug report about the default emptyvalue (then documented to be NaN, but Matlab's strread returned 0 (zero)); TMW simply changed the docs rather than fix strread.

As to the comment processing:
-----------------------------
IIRC that part has been provided by Rik. He probably doesn't remember :-)

In Matlab strread.m has no option "endofline". I've put it into the Octave version at the time as strread.m is/was the backend for textread and textscan that do have that option. It was much more convenient to handle endofline processing in strread.m.  Assessment of the "endofline" value is/was done in textread.m and textscan.m

I think we should not use the "endofline" value but silently assume a LF or CRLF during comment processing; to be overridden when "endofline" has been specified by the user. The motive is that (IIRC) strread.m's default endofline value is empty for a reason (that I do not have at hand right now).

Unlike strread, textscan returns all output from interpreting string/file contents into one cell array so I think the output in comment #2 is correct. The word boundary thing can be checked in Matlab.

Until after next November I have very little time/priority to fix strread.m.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 27 Oct 2016 04:44:20 PM UTC, comment #3: 

(Trying to fix the property changes I stepped on.)

Looking at strread.m it is correctly ignoring text between the comment character and the EOL character. The problem is that there is no default EOL character in the function unless the "endofline" property is given.

It seems a shame that textscan and strread have a lot of overlap, but suffer from different bug symptoms.

Mike Miller <mtmiller>
Group Member
Thu 27 Oct 2016 04:32:56 PM UTC, comment #2: 

Confirmed, with a couple of workarounds.

The strread function has been deprecated in Matlab, if you use textscan instead you can get the result that you are looking for, with a slight change


>> textscan ("hello #foo\nworld", "%s", "commentstyle", "#")
ans =
{
  [1,1] =
  {
    [1,1] = hello
    [2,1] = world
  }
}


I observe that textscan only recognizes commentstyle when it comes after a word boundary, that may be a separate bug.

For strread, you can use the undocumented "endofline" property to correctly get it to do what you want


>> strread ("hello#foo\nworld", "%s", "commentstyle", "shell", "endofline", "\n")
ans =
{
  [1,1] = helloworld
}


Except that this is still not quite what you were looking for. In Octave's strread, it has simply deleted all text between the comment character and the newline, and the result forms a single word.

Can you or someone test these examples in Matlab to see what changes need to be made if any?

Mike Miller <mtmiller>
Group Member
Thu 27 Oct 2016 04:31:12 PM UTC, comment #1: 

Confirmed.  The problem is general to any of the commentstyle options.  For example,


strread ("hello%foo\nworld", "%s", 'commentstyle', 'matlab'


also returns just "hello".

The strread function is in an m-file in scripts/io so it is easy enough to debug.  The problem is that the End-of-Line character in the variable eol_char is empty.  Starting at around line 325 in strread.m


  ## Remove comments in str
  if (comment_flag)
    ## Expand 'eol_char' here, after option processing which may have set value
    comment_end = strrep (comment_end, "eol_char", eol_char);


I'm adding Philip to the CC list since he understands this function better than anyone else.

Rik <rik5>
Group administrator
Thu 27 Oct 2016 07:15:04 AM UTC, original submission:  

I would expect that


strread ("hello#foo\nworld", "%s", "commentstyle", "shell")


would return

{
  [1,1] = hello
  [2,1] = world
}


but it returns "hello" instead.


Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #38830:  bug49454.cset added by philipnienhuis (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by rik5
  •  

    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 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-10-28 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2016-10-28 philipnienhuis StatusPatch Submitted Ready For Test
    2016-10-28 philipnienhuis Attached File- Added bug49454.cset, #38830
        StatusIn Progress Patch Submitted
    2016-10-28 philipnienhuis StatusNeed Info In Progress
        Assigned toNone philipnienhuis
    2016-10-27 mtmiller Summarystrread commentstyle shell doesn't work as expected strread commentstyle doesn't work as expected
    2016-10-27 mtmiller Priority5 - Normal 3 - Low
        Item GroupNone Matlab Compatibility
        StatusConfirmed Need Info
        Release4.2.0-rc2 dev
        Summarystrread commentstyle doesn't work as expected strread commentstyle shell doesn't work as expected
    2016-10-27 rik5 StatusNone Confirmed
    2016-10-27 rik5 Summarystrread commentstyle shell doesn't work as expected strread commentstyle doesn't work as expected
        Carbon-Copy- Added philipnienhuis

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code