Remove usage of old safe math and unused intsafe.h include

[skyline75489]

Summary of the Pull Request

References

#4144 introduced the chromium safe math library.

PR Checklist

• Closes Remove usages of old safe math #4153
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
• Schema updated.
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

[skyline75489]

I remember that interactivity should not be changed in this repo. But I removed the <intsafe.h> in ../../host/precomp.h so I had to add it back here to make the project compile.

[github-actions]

New misspellings found, please review:

• LONGLONG

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/5757ec679b03a4240130c3c53766c91bbc5cd6a7.txt
.github/actions/spell-check/expect/655f007265b351e140d20b3976792523ad689241.txt
.github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"AAAAA Bopomofo CParams CSV GENERATEPROJECTPRIFILE hhhh Horiz IHosted Inlines MAKEINTRESOURCEA mousemode renamer
 Reserialize rgus SGRXY tcon UDK UDKs Unfocus xe xlang "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/d1faf9f44e05f48a76241e949b605d0eea5ef23b.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"csv horiz inlines LONGLONG Renamer reserialize udk unfocus "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/d1faf9f44e05f48a76241e949b605d0eea5ef23b.txt is added to your repository...'

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

• ... misspelled, then please correct them instead of using the command.
• ... names, please add them to .github/actions/spell-check/dictionary/names.txt.
• ... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
• ... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
• ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

[j4james]

From the discussion we had in issue #4013, I got the impression we'd be getting cleaner, more readable code from the new safe math library, so I've got to say these changes are a little disappointing. What exactly are the benefits we're getting here?

And it looks like the new lib is introducing some behavioural changes too - the overflow tests look slightly different to me, and the error codes returned are different. Is this intentional? And the one behavioural change I was expecting was greater use of clamping instead of failing on overflow, but that doesn't seem to have happened.

If all we're doing is replacing one unreadable library with a slightly more unreadable library (and risking introducing new bugs in the process) is this really worth the effort?

[zadjii-msft]

This all seems sensible to me, thanks for cleaning this up!

[zadjii-msft]

@msftbot make sure @miniksa signs off on this one

[ghost]

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

• I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

[j4james]

Shouldn't all of these PtrdiffT calculations be using ptrdiff_t for the type? I don't think it's the same as LONGLONG in a 32bit build.

[j4james]

Shouldn't these E_ABORT errors be INTSAFE_E_ARITHMETIC_OVERFLOW if we want to remain compatible with the original code? It's probably not a big deal, but it's theoretically possible someone could be checking for specific error returns in some situation, especially if the return code is exposed in a public API somewhere.

[j4james]

Although I suppose if we want to get rid of the intsafe.h dependency then I guess that's not an option, unless you want to redefine the constant, or hardcode the error number. I don't know.

[miniksa]

Hum. You're right that it probably should maintain the error code in case anyone's checking specifically for it.... but I also highly doubt that anyone is given that I probably introduced it in the last few years and most things I've ever found are only checking overall success/failure from any of our API returns (if they even check at all and don't just steamroll anyway).

If we're worried about it, I don't mind redefining that constant without the header if the goal is to remove the header. But also, I don't super mind if it just becomes a different error code either...

[j4james]

Yeah OK. The more I think about it, the more I think it's probably not a big deal, especially if this code was only a fairly recent addition.

[miniksa]

@j4james had mentioned in the linked issue that we should probably replace these type conversions too to use the saturating math at the same time. If these aren't coming in from intsafe.h, where are they coming from? We should probably either do these in a batch with this PR or ensure we do a follow up to get them too for consistency.

[skyline75489]

Dang these aren't coming directly from intsafe.h but by including some win32 headers which indirectly include intsafe.h, these macros are still available. For example wil has it in https://github.com/microsoft/wil/blob/master/include/wil/safecast.h

This makes me feel that this PR isn't really doing much..

[miniksa]

Hm. Darn ok.

[skyline75489]

@j4james

I got the impression we'd be getting cleaner, more readable code from the new safe math library, so I've got to say these changes are a little disappointing.

I feel about the same. I can’t say I fully endorse this PR in terms of readability. But I still think that chromium safe math is better in terms of extensibility and modern code style.

Well, “normal” cpp code does not use HRESULT as its return value and HRESULT is exactly what THROW wants. Maybe we just can not have the best of the both worlds, huh.

I bet @miniksa can think of better reasons than me.

[j4james]

Well, “normal” cpp code does not use HRESULT as its return value and HRESULT is exactly what THROW wants. Maybe we just can not have the best of the both worlds, huh.

Well what I was hoping would happen is we'd either be using clamped math, in which case there wouldn't be any error return value, or we'd be using a form of checked math that threw an exception, in which case the error handling could be dealt with neatly in one place (instead of every single expression having an error check on it).

And once we're not doing error checking on every operation, we're in a much better position to be using regular math operators with safe math types, which I think should be much more readable than all these base::CheckXXX calls. Maybe that's asking a bit much for the first PR, but hopefully we'll be able to get there eventually.

[miniksa]

Well, “normal” cpp code does not use HRESULT as its return value and HRESULT is exactly what THROW wants. Maybe we just can not have the best of the both worlds, huh.

Well what I was hoping would happen is we'd either be using clamped math, in which case there wouldn't be any error return value, or we'd be using a form of checked math that threw an exception, in which case the error handling could be dealt with neatly in one place (instead of every single expression having an error check on it).

And once we're not doing error checking on every operation, we're in a much better position to be using regular math operators with safe math types, which I think should be much more readable than all these base::CheckXXX calls. Maybe that's asking a bit much for the first PR, but hopefully we'll be able to get there eventually.

That is true. The base::CheckXXX calls are really a kludge to deal with the existing workflow. The question is... do we take the baby step of getting everything to the chromium one first, even if it's ugly, then make a follow on to convert the ugly ones into a less ugly pattern? Or do we skip the first migration and just go straight for the second?

I know I'd find it mentally easier to do two more straight forward migrations than to have one more complicated one. But simultaneously, I can't see the core team getting to this any time soon... so I'm willing to concede whichever one is easier for the ones who are ambitious enough to do this work. If that's going straight to throwing-style calls with the bare operators on the safe math objects... (and of course catching the exception and routing it back to an HRESULT/NTSTATUS for the API call when necessary) that's fine with me.

@j4james

I got the impression we'd be getting cleaner, more readable code from the new safe math library, so I've got to say these changes are a little disappointing.

I feel about the same. I can’t say I fully endorse this PR in terms of readability. But I still think that chromium safe math is better in terms of extensibility and modern code style.

Well, “normal” cpp code does not use HRESULT as its return value and HRESULT is exactly what THROW wants. Maybe we just can not have the best of the both worlds, huh.

I bet @miniksa can think of better reasons than me.

This has just evolved from old Windows-style C code. Thus the return code focus and no real exceptions until recent-ish additions or modifications. That's why everything is so mixed. But we're ready and willing to adopt exceptions now as long as the console API functionality is maintained to the perspective of clients (within reason). Thus if you want to close this issue/PR as "dumb/pointless/outdated" and transform the work into something that is overall prettier and more faithfully represents C++ and utilizes the Chromium math libraries to their potential... I'm cool with it.

[skyline75489]

After some consideration I'm still not convinced that this is good enough.

I'll close it for now. Thanks everyone for the explanation and comments.