#286 TRangeValidator allows questionable input

[Joe Slater]

Using TRangeValidator for a TEdit control allows invalid/questionable input. Consider the example of using TRangeValidator(-23,100). One could enter "23+45-11-" in the edit field, and it would pass as valid (interpreted as the value 23). Simply entering "+" would also pass as valid (interpreted as the value 0). Using this validator appears to be a means of entering valid integer numbers within a given range, but the above examples are clearly not valid integers. The sign characters plus/minus should only be allowed at the beginning of the field. Even an empty field passes as valid which is not an integer, but I'm moving that issue to a enhancement request ticket.

[Vidar Hasfjord]

Hi Joe,

Can you test the following fix, please?

bool
TRangeValidator::IsValid(LPCTSTR s)
{
  if (!TFilterValidator::IsValid(s))
    return false;

  tistringstream is(s);
  long value = 0;
  is >> value >> std::ws;
  bool isFullyParsed = is.peek() == tistringstream::traits_type::eof();
  return isFullyParsed && value >= Min && value <= Max;
}

[Joe Slater]

Hi Vidar,

I can't test at the moment (don't ask why!).

And admittedly, I don't use streams very often, so my synopsis may be faulty…

Upon examining this code, I don't think you need to check for trailing whitespace after the stream extractor to value, since the parent class TFilterValidator doesn't allow blanks.

If the field is empty, I believe that the stream state fail will be set, and perhaps should be examined as part of the validation, because eof() by itself would be true. That is, if an empty field is not to be considered as a valid integer (and I don't).

While this should handle cases of proper validation of such things as "23+45-11-" it wouldn't (I don't think) handle cases of simply a sign given (e.g., "+"), which I don't consider a valid integer.

Additionally, my suggestion is to not have "value" as a local variable, but rather a protected data member of the class. Then the member function Transfer could be made more efficient by simply accessing this value for transference rather than duplicating the effort already performed by the IsValid function.

Furthermore, as a convenience to applications that choose not to use the Transfer member function, I would add a public member function to the class (it costs nothing to add it):

inline long GetValue (void) const { return value; }

[Joe Slater]

Not that I'm suggesting abandonment of using streams, but here is an "old-style" example implementation of IsValid (assuming value is a class data member) that should work:

bool
TRangeValidator::IsValid(LPCTSTR s)
{
  if (!TFilterValidator::IsValid(s))
    return false;

  char* p = s;

  // Skip over optional leading sign
  if ((*p == '+') || (*p == '-')) ++p;

  // Invalid if no integer present
  if (!*p) return false;

  // Get the value
  value = strtol(s, &p, 10);

  // Invalid if character follows the integer or not within range
  return !*p && value >= Min && value <= Max;
}

[Vidar Hasfjord]

Sorry, didn't see this reply before I posted mine below.

[Vidar Hasfjord]

Hi Joe, thanks for the thorough review!

[TFilterValidator obviates the] check for trailing whitespace

That's a good observation. I didn't think of that. Strictly though, we cannot rely on TFilterValidator, since TFilterValidator::ValidChars is a mutable member, possibly modified by the user (e.g. to allow white-space). So, for logical completeness, a sanity check here doesn't hurt.

[on error the] stream state [failbit] will be set, and perhaps should be examined as part of the validation

Definitely; I completely forgot this error check. Good catch!

I don't consider ["+"] a valid integer.

I tend to agree, but it is actually not straight-forward to explicitly test for this without making assumptions about the input string (stripping is needed). And, on the other hand, an argument could be made for accepting what the standard library treats as valid. I am not even sure it allows a single "+". We need to test this. So, for simplicity, I'll leave out this sanity check for now.

[reuse parsed value by adding a] protected data member

I agree very much on reuse, but not for efficiency reasons: Having the conversion code replicated in several places is bad for maintenance. However, from a design perspective, adding a member complicates the class invariant.

it costs nothing to add [GetValue]

Well, it is actually not quite straight-forward due to error handling. We will need to throw an exception if no value is yet available.

I rather not mess with the class invariant. In my view, OWLNext users should be encouraged to use the new Dialog Data Transfer framework to transfer values, and adding obscure alternatives is not helping in that sense. In any case, this is a feature request, not a bug fix.

That said, the fix is improved by factoring out the conversion code into a static helper and reusing it in Adjust and Transfer.

I have implemented this fix in [r2809]. Please test. Thanks.

[Joe Slater]

Hi Vidar,

I examined your fix code, and it appears perfect. I created a little test program using standard streams with this logic, and it works just fine.

Regarding the issue of a sole magnitude character (i.e., plus or minus) with no digits following, it does cause a failure with streams (unlike older standard C routines). So that issue is automatically handled with no further action needed. It does allow white space between the magnitude character and digits, but I don't think that's important (or even necessarily invalid) and the default behavior of this class (via TFilterValidator) won't allow blanks anyway.

I have only one comment regarding your fix…

Your comments for the ParseInteger function include:

// On success, returns the converted integer value. On failure, sets the `failbit` state of the 
// given stream and returns 0, unless exceptions are enabled for the stream, in which case an
// exception will be thrown (see std::ios_base::exceptions).

Consider the case where exceptions are not enabled, and the string "+12345-12" is parsed. The failbit will get appropriately set, but the return value will be 12345 (not 0 as your comments state). So, to be completely accurate, either change your comments accordingly or change the code for checking trailing content to reset value:

if (is.peek() != tistringstream::traits_type::eof()) {
  is.setstate(tistringstream::failbit); // No trailing content is allowed.
  value = 0l;
}

I think it's about time to close this ticket; you did good Vidar! Thank you!

[Joe Slater]

As an addendum, when releasing the documentation for this fix, as a courtesy to existing applications you may want to point out that in essence this validator now causes the field to be a "required field" such that empty fields will not be considered valid and return 0. I agree with this definition, but if others disagree and this fix is applied in conjunction with feature request #77, the ParseInteger function could check for the potentially new voRequired option and act accordingly, possibly allowing empty content to be valid and return 0. If this is done, I believe the constructors for TRangeValidator should by default however set this option to be true. This would provide a means for those in disagreement to clear the bit after construction and therefore still allow empty content. I have no need nor desire for this, but others may.

[Vidar Hasfjord]

Hi Joe, thanks for reviewing the code!

[ParseInteger should return 0 on invalid input.]

Well spotted. I have corrected the function [r2814].

this validator now causes the field to be a "required field"

That is a very good point. The old code considers blank/empty input to be valid, and for backwards compatibility, we should not change this unfortunate feature. As you suggest, we can deal with that issue in [feature-requests:#77].

I have modified the fix to restore this feature by adding a test in IsValid and changing the CHECK assertions to WARN statements in Transfer and Adjust. See [r2816].

[Joe Slater]

Looks okay Vidar!