[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Cleaning up C++ input validation of strings
From: |
Olaf Till |
Subject: |
Re: Cleaning up C++ input validation of strings |
Date: |
Mon, 16 Feb 2015 19:33:04 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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