Wed 17 Dec 2014 12:23:53 AM UTC, comment #9:
Fixed in this cset http://hg.savannah.gnu.org/hgweb/octave/rev/03067dab10ca and this addendum http://hg.savannah.gnu.org/hgweb/octave/rev/c2f4f6eb5907. Closing report.
|
Mon 15 Dec 2014 10:41:54 PM UTC, comment #8:
Rik, I think your change is OK.
I have always intentionally written "character string", but I can go with "string".
|
Sun 14 Dec 2014 12:59:44 AM UTC, comment #7:
The issue with double-quoted strings concatenated with numbers resulting in a single-quoted string is now bug #43815
|
Sun 14 Dec 2014 12:52:01 AM UTC, comment #6:
The issue with Octave:str-to-num warning never being issued is now bug #43814.
|
Sun 14 Dec 2014 12:51:15 AM UTC, comment #5:
I'm attaching a patch which makes the input validation for strings stricter. I think it is better to warn programmers when a function is being used incorrectly, rather than to attempt to proceed with bogus input, which nevertheless can be converted to a string value.
I'll split the other two issues uncovered into new bug reports.
|
Mon 30 Jun 2014 09:39:35 PM UTC, comment #4:
The 'sq_string' might be because we want to mimic Matlab, but I don't really know.
For sort, I think we should be checking that the second argument is either a numeric scalar (DIM to sort on) or is a string (MODE). It doesn't need to match the type of the first operand which might be numeric, string, or cellstr. I know I'm repeating myself, but the option string for a function really should be a string, but doesn't have to have any relationship to what kind of input the function itself acts on. Given that mixed char/numeric concatenation is promoted to character class by the interpreter people could still have strange obtuse code without generating an error message.
And yes, something is very wrong with the num-to-str warning. The warning is "on" in 3.2.4 - 4.1.X but the test code from warning_ids() doesn't produce a warning.
I think that deserves it's own bug report. Interestingly, the warning going the other way does work.
|
Mon 30 Jun 2014 08:24:15 PM UTC, comment #3:
I agree that we should try to be consistent.
Note that
is the same as
since the conversion of the mixed numeric and character array happens when the array is constructed.
For this particular example, maybe sort should warn when the two objects don't have the same type? I would guess this warning would need to be disabled in "traditional" mode.
Also, this brings up a couple of other issues. We already have an "Octave:num-to-str" warning ID that is supposed to be enabled by default, but I don't get any warning for the mixed numeric and character array construction.
Also,
produces an sq_string object, not a dq_string. I'm not sure why that choice was made.
-verbatim-
|
Mon 30 Jun 2014 08:14:36 PM UTC, comment #2:
I think the general behavior of changing transparently between numeric and char is fine. But I don't think it makes sense in many of these input validation cases. The change doesn't restrict the range of inputs that the function can accept, rather it is just restrictive of the options to the function. This may eliminate an obtuse corner case, but I don't think it would be overly restrictive.
An example with the first function on the list will probably help.
In terms of the code base it seems that about 1/2 the instances are already coded with the is_string predicate and half with the check on error_state. It would probably be best to decide on a single approach and convert the others whichever half needs it.
|
Mon 30 Jun 2014 07:23:16 PM UTC, comment #1:
Maybe it doesn't make sense, but I prefer to use string_value followed by checking for an error in case there is an octave_value type that can legitimately be converted to a string.
The automatic conversion from numbers to strings was done for what I thought was compatibility with Matlab. But maybe that's no longer needed, or at least not in as many places. I do know that it is needed for things like
but maybe what we should do is just change the way string_value works for numeric classes, and force the conversions to happen where necessary for compatibility? I think that would mean fewer places that need to be changed.
|
Sun 29 Jun 2014 10:57:20 PM UTC, original submission:
A number of input validation code blocks in libinterp are using an incorrect pattern which does not result in the correct validation of string input.
The list of occurrences is
The problem is that the code uses this sequence for input validation.
But Octave will coerce numeric arrays into strings so despite there being incorrect input the error message is never given. This code runs in Octave without an error message, but is certainly not what you want.
The fix is to use the is_string() predicate test instead. The code should look like this
These are easy changes, but there are so many of them, that I thought I would post this to the bug tracker and also put it on the list of easy bug fixes. For people who want to experiment with learning Octave's internals and how the C++ code is written this is an easy first step
|