Rectangle, Point, and Size need concerted structure #4015
Assignees
Labels
Area-CodeHealth
Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc.
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-Terminal
The new Windows Terminal.
Resolution-Fix-Committed
Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Comments
miniksa
added
Product-Conhost
ghost
added
the
Needs-Triage
DHowett-MSFT
removed
the
Needs-Triage
5 tasks
3 tasks
|
Specific reference from UiaTextRange refactor (PR #4018): |
|
To replace
This issue also stays open to track the replacements:
|
2 tasks
2 tasks
zadjii-msft
modified the milestones:
Engineering Improvements 2021,
Engineering Improvements
ghost
pushed a commit
that referenced
this issue
Jan 13, 2022
This commit makes the following changes to `til::point/size/rectangle` for the following reasons: * Rename `rectangle` into `rect` This will make the naming consistent with a later `small_rect` struct as well as the existing Win32 POINT/SIZE/RECT structs. * Standardizes til wrappers on `int32_t` instead of `ptrdiff_t` Provides a consistent behavior between x86 and x64, preventing accidental errors on x86, as it's less rigorously tested than x64. Additionally it improves interop with MIDL3 which only supports fixed width integer types. * Standardizes til wrappers on throwing `gsl::narrow_error` Makes the behavior of our code more consistent. * Makes all eligible functions `constexpr` Because why not. * Removes implicit constructors and conversion operators This is a complex and controversial topic. My reasons are: You can't Ctrl+F for an implicit conversion. This breaks most non-IDE engines, like the one on GitHub or those we have internally at MS. This is important for me as these implicit conversion operators aren't cost free. Narrowing integers itself, as well as the boundary checks that need to be done have a certain, fixed overhead each time. Additionally the lack of noexcept prevents many advanced compiler optimizations. Removing their use entirely drops conhost's code segment size by around ~6.5%. ## References Preliminary work for #4015. ## PR Checklist * [x] I work here * [x] Tests added/passed ## Validation Steps Performed I'm mostly relying on our unit tests here. Both OpenConsole and WT appear to work fine.
2 tasks
ghost
pushed a commit
that referenced
this issue
Apr 20, 2022
This commit replaces our use of `size_t` to represent VT parameters with `int32_t`. While unsigned integers have the inherent benefit of being less ambiguous and enjoying two's complement, our buffer coordinates use signed integers. Since a number of VT functions need to convert their parameters to coordinates, this commit makes the conversion easier. The benefit of this change becomes even more apparent if one considers that a number of places performed unsafe conversions of their size_t parameters to int or short already. Files that had to be modified were converted to use til wrappers instead of COORD or SMALL_RECT wherever possible. ## References This commit contains about 20% of the work for #4015. ## PR Checklist * [x] I work here * [x] Tests added/passed ## Validation Steps Performed I'm mostly relying on our unit tests here. Both OpenConsole and WT appear to work fine.
ghost
pushed a commit
that referenced
this issue
Apr 25, 2022
#4015 requires sweeping changes in order to allow a migration of our buffer coordinates from `int16_t` to `int32_t`. This commit reduces the size of future commits by using type inference wherever possible, dropping the need to manually adjust types throughout the project later. As an added bonus this commit standardizes the alignment of cv qualifiers to be always left of the type (e.g. `const T&` instead of `T const&`). The migration to type inference with `auto` was mostly done using JetBrains Resharper with some manual intervention and the standardization of cv qualifier alignment using clang-format 14. ## References This is preparation work for #4015. ## Validation Steps Performed * Tests pass ✅
3 tasks
This was referenced May 13, 2022
ghost
pushed a commit
that referenced
this issue
May 16, 2022
This commit is one of the more difficult rewrites that were necessary as part of #4015, but still simple enough that it can be done as a separate commit. The search for the `lastNonSpace` was replaced with a simpler `std::string_view::find_last_not_of`. ## Validation Steps Performed ConPTY appears to work ✅
ghost
pushed a commit
that referenced
this issue
May 16, 2022
This commit includes various minor improvements to til::hash/point/size/rect which accumulated while working on #4015. * Allow xvalue containers and non-`size_t` indices in `til::at`. * `til::as_unsigned` can be used to reinterpret a potentially signed integer as a unsigned one. This can potentially enable some optimizations as no sign extension is needed anymore. `til::hash` can make use of this to drop about 20% of the hashing of signed integers <= 32 bit. On x86 this translates to a `mov` (virtually no latency) or no instructions at all, instead of requiring a `movsx` (some latency) for sign extension. * `til::point` operators that prefer mutability. This is a opinionated change, but it follows the STL style beter and generates less assembly. * Simpler `rect` scale_up/down and `size` divide_ceil. `scale_up` will not depend on the operator header anymore. `scale_down` / `divide_ceil` can be implemented without checked numerics, so I did. It also follows the related GdiEngine code better now, which makes me confident that we can replace GdiEngine's code with this. * Removal of rect-size-shift operators. They were only used in DxEngine and confusing as they weren't commutative. Adding and then subtracting a size from a rect (and vice versa) didn't do what you'd intuitively think it'd do. The code was replaced with addition and clamps in DxEngine. * Various unsafe `as_` casts for point/size/rect. This will aid the migration in #4015. ## Validation Steps Performed * Vertical scrolling works in `DxEngine` ✅
ghost
added
Resolution-Fix-Committed
ghost
pushed a commit
that referenced
this issue
Jun 3, 2022
Previously this project used a great variety of types to present text buffer coordinates: `short`, `unsigned short`, `int`, `unsigned int`, `size_t`, `ptrdiff_t`, `COORD`/`SMALL_RECT` (aka `short`), and more. This massive commit migrates almost all use of those types over to the centralized types `til::point`/`size`/`rect`/`inclusive_rect` and their underlying type `til::CoordType` (aka `int32_t`). Due to the size of the changeset and statistics I expect it to contain bugs. The biggest risk I see is that some code potentially, maybe implicitly, expected arithmetic to be mod 2^16 and that this code now allows it to be mod 2^32. Any narrowing into `short` later on would then throw exceptions. ## PR Checklist * [x] Closes #4015 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed Casual usage of OpenConsole and Windows Terminal. ✅
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
OK, as I'm refactoring, there's a few problems:
size_t,long, andSHORTbased versions of representations of rectangles, points, and sizes.SIZE. It should probably be passed as one sensible struct because the ordering of these parameters keeps getting reversed too.COORD. x is width and y is height for size usage.We're also going to want to be able to expand most of our values to reach an "infinite" scrollback.
My proposal here would be:
typeslibrary, we should create:xandylike Windows SDK 'POINT' but issize_t.POINTsafely.COORDsafely.size_t.SIZEsafely.COORDsafely.Pointand aSize.RECTsafely.SMALL_RECTsafely.Then walk through the code and update to use these things.
The text was updated successfully, but these errors were encountered: