CopyPasteManagerTest fails to test all values

[rudyhuyn]

CopyPasteManager unit tests were not fully run, the first item of arrays were never tested. (Luckily, the not-tested values were ok).

Description of the changes:

while(--size)
the following code won't run the code in the loop with size==0.

Instead we should use:
while(--size >= 0)

Before (first value not tested):

After (the test fails as expected):

[rudyhuyn]

Do we need to open a Issue ticket when it doesn't concern the app itself?

[HowardWolosky]

Woah. Nice catch, Rudy.

I detest pre-increment/decrement. In my experience, more times than not, their usage leads to bugs. Whenever I see pre-increment/decrement, Case in point. I have to slow down significantly when reading the code to understand why that part of the code is so special.

Your fix is correct. I'd much prefer it though if you simply switched it to being a for loop with an inline-declared int i iterator (e.g. for (int i = 0; i < size; i++)). You'd have to update the access at 31, 40 and 457, but I think it would be totally worth it for just code clarity. Modifying size to use as an iterator is a really bad pattern, as it gives the completely wrong impression on what is going on...especially at 457. Think you can do one additional update to simply switch this to a for loop instead?

As for whether or not this needs an issue -- I'm working on getting the updated contributing guidance up by tomorrow. We've internally settled on keeping it consistent that there should be an approved issue for every PR that is reviewed. The bar for getting an issue approved will be lower for changes that don't impact what an end-user will see. PR's that are out before the updated contribution documentation is committed will be grandfathered in on a case-by-case basis. This one will be grandfathered in to not need the issue.

[rudyhuyn]

Woah. Nice catch, Rudy.

I detest pre-increment/decrement. In my experience, more times than not, their usage leads to bugs. Whenever I see pre-increment/decrement, Case in point. I have to slow down significantly when reading the code to understand why that part of the code is so special.

Your fix is correct. I'd much prefer it though if you simply switched it to being a for loop with an inline-declared int i iterator (e.g. for (int i = 0; i < size; i++)). You'd have to update the access at 31, 40 and 457, but I think it would be totally worth it for just code clarity. Modifying size to use as an iterator is a really bad pattern, as it gives the completely wrong impression on what is going on...especially at 457. Think you can do one additional update to simply switch this to a for loop instead?

As for whether or not this needs an issue -- I'm working on getting the updated contributing guidance up by tomorrow. We've internally settled on keeping it consistent that there should be an approved issue for every PR that is reviewed. The bar for getting an issue approved will be lower for changes that don't impact what an end-user will see. PR's that are out before the updated contribution documentation is committed will be grandfathered in on a case-by-case basis. This one will be grandfathered in to not need the issue.

I can't more agree, this is the kind of "I try to optimize my code" but in fact, it adds bugs/makes the code harder to read, create an .exe less optimized because the compiler had more troubles to optimize the code".

[HowardWolosky]

Definitely more clear, thanks for the update.

[HowardWolosky]

Closing (and will immediately re-open) in order to kick off the PR build again.