[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/
- [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification, Júlio Hoffimann Mendes, 2013/03/09
- [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification, Philip Nienhuis, 2013/03/09
- [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification, Júlio Hoffimann Mendes, 2013/03/09
- [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification, Philip Nienhuis, 2013/03/10
- [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification, Júlio Hoffimann Mendes, 2013/03/10
- [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification, Philip Nienhuis, 2013/03/10
- [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification, Philip Nienhuis, 2013/03/12
- [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification,
Philip Nienhuis <=
- [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification, Júlio Hoffimann Mendes, 2013/03/17
- [Octave-patch-tracker] [patch #7969] Organized textread.m for better error notification, Philip Nienhuis, 2013/03/18