octave-patch-tracker
[Top][All Lists]
Advanced

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

[Octave-patch-tracker] [patch #7969] Organized textread.m for better err


From: Philip Nienhuis
Subject: [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification
Date: Sat, 16 Mar 2013 23:52:24 +0000
User-agent: Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0 Iceape/2.16

Follow-up Comment #7, patch #7969 (project octave):

As it stands, your patch line 60:
 "if (headerlines && isindex (headerlines+1, numel (varargin)))"
won't work reliably:

1. It would introduce a bug as the number of user-specified headerlines bears
no relation to the number of args to textread.m (=numel(varargin)+2). It may
be better to omit this 2nd arg to isindex() completely. Or did you
misunderstand how isindex() works? 
Similar issue further below in your patch.

2. isindex (headerlines_value) would rule out a zero value which would still
be acceptable (remember these headerlines values could have been computed in
other scripts that invoke textread.m). The only thing is that a zero value
shouldn't be fed to fskipl(), but that is a textread coder's issue, not a
textread user issue.

As to informative-, warning- and error messages:
- Your patch line 47 doesn't explain to user why there are no lines to read.
- Similar reasoning for your patch line 70 (about headerlines value).
- In your patch line 73: ' headerlines' must be a /nonnegative/ number (inform
the user of acceptable headerlines values)

We've already discussed EOL issues.

BTW in the textscan.m stanza of your patch there's still a call to textread
instead of textscan itself. I'll try to fix that tonight.

My plan:
Using your patch as a guide I'll clean up textread.m as far as reasonable, put
the result on the tracker so you can have a look at it, and if you agree on
it, I'll push it attributing to you. As textread.m works OK at the moment I
can't assign it a very high priority.
Of course you can also come up with a revised patch. But be wary, it seems
that textread's test suite doesn't catch all the corner cases, some more tests
may be needed.


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/patch/?7969>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/




reply via email to

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