Replaced comparison with -1 w/ equivalent comparison to 0

[gfoidl]

This allows the cpu to fuse the compare and jump, thus cpu's backend has a little bit less work to do.
It's a micro-optimization, but the code-change is quite trivial and easy to follow.

[dotnet-policy-service]

Thanks for your PR, @@gfoidl. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[javiercn]

I'm not a fan of this type of change; it makes the code less "idiomatic" and we don't have numbers to back that it produces a significant improvement. I understand the CPU will use a better instruction, but the impact is likely negligible in most cases and it definitely takes an extra split second to reason when you are looking at code in things like IndexOf and so on and it does >= instead of comparing against -1. This is just my personal opinion.

[mgravell]

I'm going to be a little contrarian - on the "idiomatic" point of @javiercn ; it could just be the way my brain works, but in all of these methods I don't think of -1 as the sentinel; right-or-wrong: I think of "negative" as the sentinel, so this is absolutely "idiomatic" versus any of the code I write. I do share the thought on unevidenced performance changes, though; in a quick test the relevant change here between < 0 and == -1 seems to be:

    L0000: test ecx, ecx
    L0002: jge short L001e

vs

    L0000: cmp ecx, 0xffffffff
    L0003: jne short L001f

Which of those is faster? I don't know, but the difference is probably marginal and dwarfed by the thing that returns the value (IndexOf etc).

[martincostello]

+1 on the "is negative" view - that's the correct semantic meaning. I'm sure if for some reason -2 was ever returned, a lot of code would be broken because the condition "went the wrong way".

[gfoidl]

Which of those is faster?

    L0000: test ecx, ecx
    L0002: jge short L001e

is faster.

• test and jge are executed as only 1 instruction at the cpu-level
• code size is a bit smaller, and almost always less code size is better

It won't change the needle much perf-wise, but a tad here and there can sum up.

I don't think of -1 as the sentinel; right-or-wrong: I think of "negative" as the sentinel

That's actually the way to think here. -1 was just chosen as it's the first negative number.

[mgravell]

which means "correctness" may be a better and easier hammer to justify this PR, rather than perf ;p Working on a BDN, though.

[javiercn]

All in all, the comment about the "negative" vs a concrete number (-1) is fair, but the reality is that the implementation chose to return -1 (and that can't change without breaking the world) and the docs and everything explicitly call out -1 as opposed to a negative number https://learn.microsoft.com/en-us/dotnet/api/system.string.indexof?view=net-9.0.

The other aspect of this is that I doubt any coding assistant will go for < 0 or similar as opposed to == or != -1 and that means over time the codebase will end up with mixed approaches.

I'm just adding this for completeness, I want to be 100% clear that I don't care enough about this for this to be a problem, I'm just cautioning of going down the road of making these types of changes without backing numbers when they just make the code less legible for a percentage of people

[mgravell]

quick and dirty BDN here

Results inconclusive - I think we're talking about the rounding noise:

Method	IsHit	Mean	Error	StdDev
Negative	False	53.85 ns	0.724 ns	0.677 ns
MinusOne	False	50.83 ns	0.837 ns	0.783 ns
Negative	True	42.69 ns	0.425 ns	0.397 ns
MinusOne	True	43.62 ns	0.231 ns	0.216 ns

Again, "correctness" may be the better hammer here.