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

 
 

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

Submitted by:  None
Submitted on:  Thu 27 Oct 2016 07:15:04 AM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 3 - LowItem Group: Matlab Compatibility
Status: FixedAssigned to: Philip Nienhuis <philipnienhuis>
Originator Name: Originator Email: -unavailable-
Open/Closed: ClosedRelease: 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 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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project 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.

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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project Administrator
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

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

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>
Project Administrator
Thu 27 Oct 2016 04:31:12 PM UTC, comment #1:

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

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

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

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

I would expect that

would return

but it returns "hello" instead.

Anonymous

 

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

Attach File(s):
   
   
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
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by rik5
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 16 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 28 Oct 2016 05:47:26 PM UTCrik5StatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Fri 28 Oct 2016 01:23:51 PM UTCphilipnienhuisStatusPatch Submitted=>Ready For Test
    Fri 28 Oct 2016 10:15:42 AM UTCphilipnienhuisAttached File-=>Added bug49454.cset, #38830
      StatusIn Progress=>Patch Submitted
    Fri 28 Oct 2016 09:16:12 AM UTCphilipnienhuisStatusNeed Info=>In Progress
      Assigned toNone=>philipnienhuis
    Thu 27 Oct 2016 04:44:20 PM UTCmtmillerSummarystrread commentstyle shell doesn't work as expected=>strread commentstyle doesn't work as expected
    Thu 27 Oct 2016 04:32:56 PM UTCmtmillerPriority5 - 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
    Thu 27 Oct 2016 04:31:35 PM UTCrik5StatusNone=>Confirmed
    Thu 27 Oct 2016 04:31:12 PM UTCrik5Summarystrread 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.1-cleanup1