octave-maintainers
[Top][All Lists]
Advanced

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

Re: Cleaning up C++ input validation of strings


From: Rik
Subject: Re: Cleaning up C++ input validation of strings
Date: Tue, 17 Feb 2015 09:32:06 -0800

On 02/16/2015 08:14 AM, Carnë Draug wrote:
> On 16 February 2015 at 15:43, Rik <address@hidden> wrote:
>> On 02/16/2015 04:43 AM, Carnë Draug wrote:
>>> On 14 February 2015 at 01:09, Rik <address@hidden> wrote:
>>>> 2/13/15
>>>>
>>>> There seem to be multiple instances where we check whether an input is a
>>>> string, then extract a string_value, and then check the error_status to
>>>> make sure that the string_value was extracted correctly.  This seems
>>>> redundant and possibly a lazy copying of a correct pattern.
>>>>
>>>> If something might not be of type A, but possibly could be converted to
>>>> type A, then asking for A_value () and checking the error_status is the
>>>> correct thing to do.  In this case, If something is determined to be a
>>>> string, can the string_value() call really fail?
>>>>
>>>> Here is an example from lu.cc
>>>>
>>>> -- Code --
>>>> if (args(n).is_string ())
>>>>   {
>>>>     std::string tmp = args(n++).string_value ();
>>>>
>>>>     if (! error_state)
>>>>       {
>>>>         if (tmp.compare ("vector") == 0)
>>>>           vecout = true;
>>>>         else
>>>>           error ("lu: unrecognized string argument");
>>>>       }
>>>>   }
>>>> -- End Code --
>>>>
>>>> I propose simplifying to
>>>>
>>>> -- Code --
>>>> if (args(n).is_string ())
>>>>   {
>>>>     std::string tmp = args(n++).string_value ();
>>>>
>>>>     if (tmp.compare ("vector") == 0)
>>>>       vecout = true;
>>>>     else
>>>>       error ("lu: unrecognized string argument");
>>>>   }
>>>> -- End Code --
>>>>
>>>> Are there any objections?
>>> I was under the impression that we should avoid using is_* functions as
>>> much as possible and instead check error_state.  Following this logic,
>>> should not the recommendation be:
>>>
>>> -- Code --
>>>     std::string tmp = args(n).string_value ();
>>>     if (! error_state)
>>>       {
>>>         n++;
>>>         if (tmp.compare ("vector") == 0)
>>>           vecout = true;
>>>         else
>>>           error ("lu: unrecognized string argument");
>>>       }
>>> -- End Code --
>>>
>>> Carnë
>> That is the correct general pattern, but not for the input validation of
>> string option arguments.  As an example, in this particular case the option
>> string "vector" means do something different.  If you just ask for a
>> string_value then the double matrix
>>
>> [118   101    99   116   111   114]
>>
>> which spells out "vector" would also be accepted.  But if you are supplying
>> numeric values like this you haven't read the docstring and are probably
>> misusing the function.  We can't program Octave with a full Artificial
>> Intelligence agent, but we would like to cut short as quickly as possible
>> any Garbage In/Garbage Out calculations by validating the inputs as closely
>> as possible.
>>
> But isn't that the whole point of the num-to-str warning?  The
> string_value() method even has a force option so as to not trigger
> the warning.  Shouldn't error_state be set if force is false?
>
> Carnë

The people who use Octave generally are not programmers, but wear a
different hat such as chemist, engineer, biologist, etc.  They need Octave
to be a useful tool immediately which does their calculations and doesn't
lead them astray.  Octave should help them along by issuing specific and
relevant error messages.  I remember coding early on in BASIC and the only
error it would give was "Syntax Error".  I don't even think it mentioned a
line number.  This was completely unhelpful and the opposite of what we want.

I'm sure I haven't gotten everywhere, but I have been trying to improve the
input validation in Octave for the reasons above.  In cases where the
function is expecting an option string, but gets something else, Octave
should issue an error and stop so the problem can be corrected.  Relying on
a warning isn't the end of the world, but is less desirable.  The warning
can scroll off the screen, and people pay less attention to warnings, or
they might have turned off the num-to-str warning entirely.

In the cases I mean to clean up, the is_string() function is already in
place to do this:

if (arg.is_string ())
...
else
  error ("OPTION must be a string")

It's in the ellipsis code that I don't think it is worth checking
error_state because I already know the arg is a string and string_value
should therefore be able to extract a value without problems and without
the need to check error_state.

--Rik





reply via email to

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