C++ comparison expression bug

Question

There's something wrong with this code, but I can't find what's causing it.

bool Parser::validateName(std::string name) {
    int pos = name.find(INVALID_CHARS);               //pos is -1, 
    bool result = ((name.find(INVALID_CHARS)) < 0);   //result is false
    //That was weird, does that imply that -1 >= 0?, let's see
    result = (pos < 0)                                //result is true
    result = ((name.find(INVALID_CHARS)) == -1)       //result is true
    result = (-1 < 0)                                 //result is true
    ...
}

Why is the result false at the second line. Is there something I'm not seeing?

note that additionally using the plural form in `INVALID_CHARS` makes one wonder if this is a string full of invalid chars, and whether you don't want something like `find_first_of` — PlasmaHH, Dec 13 '12 at 15:57
npos is not -1, it is 0xFFFFFFFF. That expression will always be false... — joy, Dec 13 '12 at 16:00
FWIW, my compiler gives "warning: comparison of unsigned expression < 0 is always false" — R. Martinho Fernandes, Dec 13 '12 at 16:02
@R.MartinhoFernandes Yes, I don't know why compiler didn't warn me... I guess I'll check that makefile. — Guido Tarsia, Dec 13 '12 at 16:06
@R.MartinhoFernandes which compiler are you using? I'm using GNU g++ 4.6.3 and didn't get a warning for that line, but if I compare my own unsigned vs signed variables, compilers does give that warning. — Guido Tarsia, Dec 13 '12 at 16:11
@neagoegab: 0xffffffff just happens to be the representation of -1 casted to a 32bit unsigned integer in your implementation. The standard defines npos to be size_type(-1) and size_type to be an implementation defined unsigned integer. Most implementations make this be size_t which on 64bit architectures is often a 64bit unsigned integer. But what its representation is exactly is not specified, and also of no matter since the right way to do things is to compare to std::string::npos — PlasmaHH, Dec 13 '12 at 16:16
@PlasmaHH of course, but I wanted to keep it simple... whithout to many explanation so he could dig a little bit. — joy, Dec 14 '12 at 09:00

score 9 · Accepted Answer · answered Dec 13 '12 at 15:54

9

std::string::find returns std::string::npos which is of type std::string::size_type which is defined to be an implementation defined unsigned integer. Unsigned integers are never smaller than 0.

You should always compare against std::string::npos to check whether std::string::find found something or not.

answered Dec 13 '12 at 15:54

PlasmaHH

15,673
5
44
57

OK, I'll do that, and I guess your right, but if npos is of type unsigned, how did `pos` have a value a `-1` after runnning the first line. – Guido Tarsia Dec 13 '12 at 15:58
@Erandros `size_t p1= -1; int p2 = p1; std::cout< – user93353 Dec 13 '12 at 15:59
Why does `((name.find(INVALID_CHARS)) == -1)` compare to `true` if it uses unsigned integers? – TeaOverflow Dec 13 '12 at 16:00
1

@Erandros The right way to do this is something like `auto location = name.find(INVALID_CHARS); location != std::string::npos` -- if you lack the auto feature, use `std::string::size_type`. Pos has a value of `-1` because you cast an unsigned type to a signed type, which can result in a negative value even if the original value wasn't negative (it is common for `unsigned int(-1)` to be the max value of `unsigned int`). – Yakk - Adam Nevraumont Dec 13 '12 at 16:00
@Evgeni because "`unsigned`" doesn't mean "positive". – Yakk - Adam Nevraumont Dec 13 '12 at 16:01
@Erandros - http://stackoverflow.com/questions/5416414/signed-unsigned-comparisons – user93353 Dec 13 '12 at 16:02

score 2 · Answer 2 · answered Dec 13 '12 at 16:04

std::string::find returns std::string::npos when it does not find the requested item. According to the Standard (§ 21.4/5):

static const size_type npos = -1;

But see that string::size_type is usually unsigned int; this means that -1 is converted into its unsigned equivalent. Usually, 0xFFFF, which is the maximum value for unsigned int.

In your second line:

bool result = ((name.find(INVALID_CHARS)) < 0);

you are comparing two unsigned int values (0xFFFF and 0), so this returns false. On the other hand, in your fourth line:

result = ((name.find(INVALID_CHARS)) == -1)

you have an unsigned int and an int, so promotion rules apply and the unsigned int is converted into an int; as we saw before, the signed equivalen of npos is always -1, so this returns true.

C++ comparison expression bug

2 Answers2