Re: Cleaning up C++ input validation of strings

[Olaf Till]

On Mon, Feb 16, 2015 at 04:14:30PM +0000, 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?

My 2 cents: If the concept is (?):

- ..._value(): convert anything which can be converted,

- is_...(): check for a special case,

then treatment of special cases by ..._value(), be it by a warning or
by setting error_state, seems to be an inconsistency with regard to
this concept, which I'd tend to avoid.

BTW, the more "if (error_state)" code is introduced (or left in) now,
the more will possibly have to be converted later to use exception
handling ...

Olaf

-- 
public key id EAFE0591, e.g. on x-hkp://pool.sks-keyservers.net

signature.asc
Description: Digital signature