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

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.

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).

+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".

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.

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

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

quick and dirty BDN here

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

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

The benchmark above is clearly dominated by the cost of IndexOf and not the comparison 😉.
Perf-wise it's about cpu's ability in the decoder (front-end) to produce less instructions that are actually executed in the back-end by fusing them together. This won't give huge perf-wins, but it's tiny little bit, that can sum up.
In runtime similar changes were done (I can't find the PRs for this at the moment, there wasn't one huge - rather it was done gradually).

The benchmark above is clearly dominated by the cost of IndexOf and not the comparison

Which was sort of intentional.

In runtime similar changes were done

I think we should try to find some of those examples - there's probably a lot of prior discussion we can lean on.

I don't think we should make code less clear for such a minor perf improvement.

we should try to find some of those examples

https://grep.app/search?f.lang=C%23&f.repo=dotnet%2Fruntime&f.repo.pattern=dotnet%2Fruntime&q=indexof

I started in runtime with this several years ago, now it's used widespread over there and almost all new code uses a comparison w/ 0, so why not here too?

make code less clear

Well, that's IMO in the eye of the beholder.

The code isn't less clear with

int idx = IndexOf(...);
-if (idx == -1)
+if (idx >= 0)

-1 is an artificial sentinel, that denotes that no index was found.
On the other hand a found index can only be in [0, n) where n is the lenght of the set.
So code either way is correct and fine, but a comparison with 0 allows CPUs optimizations and is less machine code, thus I suggest to compare to 0.

I see this as a change to improve readability rather than a change to performance. If the change has actually improved the performance(assuming it to be micro-optimization), it would be good to see any evidence suggesting the improvement.

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.