Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove usages of old safe math #4153

Open
miniksa opened this issue Jan 8, 2020 · 3 comments
Open

Remove usages of old safe math #4153

miniksa opened this issue Jan 8, 2020 · 3 comments
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Conpty For console issues specifically related to conpty

Comments

@miniksa
Copy link
Member

miniksa commented Jan 8, 2020

There are approximately 79 uses of old safe math. (a.k.a. the LongAdd, ShortAdd, and friends from intsafe.h).

This task represents removing them and transitioning them to an appropriate safe/saturating math function from the chromium safe math library introduced in #4144.

After removing usages of the old safe math functions, also remove the intsafe.h header.

Files with old safe math as of this writing:

  • TerminalControl: TSFInputControl.cpp
  • TerminalCore: TerminalDispatch.cpp
  • TerminalCore: TerminalSelection.cpp
  • Host: directio.cpp
  • RendererGdi: invalidate.cpp
  • RendererGdi: math.cpp
  • RendererGdi: paint.cpp
  • RendererVt: paint.cpp
  • TerminalAdapter: adaptDispatch.cpp
  • Types: viewport.cpp
  • Types: WindowUiaProviderBase.cpp
@miniksa miniksa added Product-Conhost For issues in the Console codebase Help Wanted We encourage anyone to jump in on these. Product-Conpty For console issues specifically related to conpty Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Jan 8, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 8, 2020
@miniksa miniksa removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 8, 2020
@miniksa
Copy link
Member Author

miniksa commented Jan 8, 2020

If @j4james doesn't take this, whomever takes this should consult with @j4james on the ones in adaptDispatch.cpp because they should likely be saturating math instead of failing when exceeding bounds as they are today.

@j4james
Copy link
Collaborator

j4james commented Jan 8, 2020

I don't mind doing this eventually, but I have bunch of stuff in progress that I would like to try and get finished first. If anyone else wants to take it on, though, I would recommend waiting at least until #3628 is merged, because there is a fair amount of safe math code in AdaptDispatch that will be nuked by that PR, so you'll be wasting your time if you're trying to refactor that.

On a similar note, I expect issue #3849 will end up replacing most, if not all, of the TerminalDispatch code. That said, it may be a long time before that happens, so it's possibly still worth cleaning up the existing code in the mean time.

@zadjii-msft zadjii-msft added this to the Console Backlog milestone Jan 9, 2020
@j4james
Copy link
Collaborator

j4james commented Jan 9, 2020

One other thing I wanted to add: there is another category of intsafe routines which I think isn't covered by the 79 uses mentioned above, and that is the type conversion functions, e.g. SizeTToShort. In the VT code, I think many of those would probably need to be converted to something like a saturated_cast when using the new chromium lib.

ghost pushed a commit that referenced this issue Jan 16, 2020
## Summary of the Pull Request

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #4013 
* [x] I work here.
* [x] Existing tests should be OK. Real changes, just adding a lib to use.
* [x] Couldn't find any existing docs about intsafe.
* [x] Am core contributor.

## Detailed Description of the Pull Request / Additional comments
* [x] Can we remove min/max completely or rename it in the two projects where it had to be reintroduced? This is now moved into #4152 
* [x] How many usages of the old safe math are there? **79**
* [x] If not a ton, can we migrate them here or in a follow on PR? This is now moved into #4153

Files with old safe math:
- TerminalControl: TSFInputControl.cpp
- TerminalCore: TerminalDispatch.cpp
- TerminalCore: TerminalSelection.cpp
- Host: directio.cpp
- RendererGdi: invalidate.cpp
- RendererGdi: math.cpp
- RendererGdi: paint.cpp
- RendererVt: paint.cpp
- TerminalAdapter: adaptDispatch.cpp
- Types: viewport.cpp
- Types: WindowUiaProviderBase.cpp

## Validation Steps Performed
@ghost ghost added In-PR This issue has a related PR and removed In-PR This issue has a related PR labels Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants