0002356: strcmp function fails with comparing comparing empty strings with numbers

Viewing Issue Simple Details [ Jump to Notes ]

[ View Advanced ] [ Issue History ] [ Print ]

Category

Severity

Reproducibility

Date Submitted

Last Update

0002356

[Quercus]

major

always

01-22-08 15:20

01-23-08 22:27

Reporter

rvt

View Status

public

Assigned To

nam

Priority

normal

Resolution

fixed

Status

closed

Product Version

Summary

0002356: strcmp function fails with comparing comparing empty strings with numbers

Description

strcmp fails when comparing numbers and empty strings.

Example:

strcmp('0', '') returns 0 while it should return 1

Additional Information

ey Guys,
(all based on latest SVN version of 19 jan 2008)

I created the following test:

resin Results:
strcmp('a', 'a') : 0
strcmp('b', 'a') : 1
strcmp('a', 'b') : -1
strcmp('', '0') : 0
strcmp(' ', '0') : 0
strcmp('', 'a') : -1
strcmp('', '') : 0
strcmp('0', '') : 0
strcmp('00', '') : 0
strcmp('10', '') : 1
strcmp('1', '1') : 0
strcmp('1', '2') : -1
strcmp('2', '1') : 1
strcmp('-1', '-2') : 0
strcmp('-2', '-1') : -1
strcmp('2', '2') : 1

mod_php results:
strcmp('a', 'a') : 0
strcmp('b', 'a') : 1
strcmp('a', 'b') : -1
strcmp('', '0') : -1
strcmp(' ', '0') : -16
strcmp('', 'a') : -1
strcmp('', '') : 0
strcmp('0', '') : 1
strcmp('00', '') : 2
strcmp('10', '') : 2
strcmp('1', '1') : 0
strcmp('1', '2') : -1
strcmp('2', '1') : 1
strcmp('-1', '-2') : 0
strcmp('-2', '-1') : -1
strcmp('2', '2') : 1

As you can see all evaluations between empty strings and numbers are wrong,
looking at the java code (see below the mail) it's clear that there is an test done
to see if both values can evaluate to a number (if (isNumberConvertible() || rValue.isNumberConvertible()) { )

However apparently an empty string or string with spaces is seen as a number 0 (nul).

To solve this issue properly would you feel that

1) isNumberConvertible() should return false on empty strings

2) isNumberConvertible() should return true on empty strings, but the cmp functions needs to get changed.

This is what I did to solve my problem for now, when I comment out the complete evaluation for numbers and do a true string compare,
this would be the output of resin (see code after:):

strcmp('a', 'a') : 0
strcmp('b', 'a') : 1
strcmp('a', 'b') : -1
strcmp('', '0') : -1
strcmp(' ', '0') : -1
strcmp('', 'a') : -1
strcmp('', '') : 0
strcmp('0', '') : 1
strcmp('00', '') : 1
strcmp('10', '') : 1
strcmp('1', '1') : 0
strcmp('1', '2') : -1
strcmp('2', '1') : 1
strcmp('-1', '-2') : 0
strcmp('-2', '-1') : -1
strcmp('2', '2') : 1

Now somebody did decide to check for numbers and do a comparison from there,
may be it was speed reasons? or any other behafure I mist have missed?

Let me know how you feel about this so I can propose a proper patch and an entry in bug trac later on,

kind regards,
Ries van Twisk

before:
  public int cmp(Value rValue)
  {
    if (isNumberConvertible() || rValue.isNumberConvertible()) {
      double l = toDouble();
      double r = rValue.toDouble();

      if (l == r)
        return 0;
      else if (l < r)
        return -1;
      else
        return 1;
    }
    else {
      int result = toString().compareTo(rValue.toString());

      if (result == 0)
        return 0;
      else if (result > 0)
        return 1;
      else
        return -1;
    }
  }

after (always compare true strings):
  public int cmp(Value rValue)
  {

      int result = toString().compareTo(rValue.toString());

      if (result == 0)
        return 0;
      else if (result > 0)
        return 1;
      else
        return -1;
  }

Attached Files

Relationships

Notes
(0002674) nam 01-23-08 22:27	php/110h

Issue History
Date Modified	Username	Field	Change
01-22-08 15:20	rvt	New Issue
01-23-08 22:27	nam	Status	new => assigned
01-23-08 22:27	nam	Assigned To	=> nam
01-23-08 22:27	nam	Status	assigned => closed
01-23-08 22:27	nam	Note Added: 0002674
01-23-08 22:27	nam	Resolution	open => fixed
01-23-08 22:27	nam	Fixed in Version	=> 3.1.5

Mantis 1.0.0rc3[^]

29 total queries executed.
26 unique queries executed.