Skip to content

Remove usages of old safe math #4153

Open

Description

@miniksa

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

Activity

  1. added
    Product-ConhostFor issues in the Console codebase
    Help WantedWe encourage anyone to jump in on these.
    Product-ConptyFor console issues specifically related to conpty
    Issue-TaskIt's a feature request, but it doesn't really need a major design.
    Area-CodeHealthIssues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc.
    on Jan 8, 2020
  2. ghost added
    Needs-TriageIt's a new issue that the core contributor team needs to triage at the next triage meeting
    on Jan 8, 2020
  3. removed
    Needs-TriageIt's a new issue that the core contributor team needs to triage at the next triage meeting
    on Jan 8, 2020
  4. miniksa commented on Jan 8, 2020

    @miniksa
    ContributorAuthor

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

  5. j4james commented on Jan 8, 2020

    @j4james
    Collaborator

    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.

  6. j4james commented on Jan 9, 2020

    @j4james
    Collaborator

    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.

  7. ghost added a commit that references this issue on Jan 16, 2020
  8. ghost added
    In-PRThis issue has a related PR
    on Dec 3, 2020
  9. ghost removed
    In-PRThis issue has a related PR
    on Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Area-CodeHealthIssues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc.Help WantedWe encourage anyone to jump in on these.Issue-TaskIt's a feature request, but it doesn't really need a major design.Product-ConhostFor issues in the Console codebaseProduct-ConptyFor console issues specifically related to conpty

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions