Various improvements for til::hash/point/size/rect #13093

lhecker · 2022-05-13T00:33:47Z

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 Rectangle, Point, and Size need concerted structure #4015.

Validation Steps Performed

Vertical scrolling works in DxEngine ✅

DHowett · 2022-05-13T00:40:41Z

src/inc/til/at.h

-    template<class T, std::enable_if_t<!details::is_span<T>::value, int> = 0>
-    constexpr auto at(T& cont, const size_t i) -> decltype(cont[cont.size()])
+    template<typename T, typename I>
+    constexpr auto at(T&& cont, const I i) noexcept -> decltype(auto)


Surprisingly C++14. Using auto for parameter types is C++20 though...

DHowett · 2022-05-13T18:39:20Z

src/inc/til/at.h

-    template<class T, std::enable_if_t<!details::is_span<T>::value, int> = 0>
-    constexpr auto at(T& cont, const size_t i) -> decltype(cont[cont.size()])
+    template<typename T, typename I>
+    constexpr auto at(T&& cont, const I i) noexcept -> decltype(auto)


Alright so decltype(auto) here (instead of auto) makes sure that this can return an Elem& (and will not return an Elem instead)?

Yep! We could also write constexpr decltype(auto) ... but trailing types are technically cooler. 😅

DHowett · 2022-05-13T18:41:32Z

src/inc/til/point.h

+        POINT* as_win32_point() noexcept
+        {
+#pragma warning(suppress : 26490) // Don't use reinterpret_cast (type.1).
+            return std::launder(reinterpret_cast<POINT*>(this));


first time i have ever seen somebody introduce a launder in the wild. Now, can you explain why it is necessary here?

My understanding is:
C++ has reinterpret_cast but it's pretty much entirely useless.
You want to make an unsafe cast, because you know that the bitwise representation between both types is identical? Great. Because you can't. The language standard doesn't allow this and converting a til::point to POINT via reinterpret_cast is invalid no matter what.

std::launder is the next best tool in the language to say "I want what I want" even if it's purpose might not be for this. The spec says given std::launder(p): "All bytes of storage that would be reachable through the result are reachable through p". That's something I want: When you use that laundered pointer and make changes, I want those changes to be reachable through this as well. But my hope is not to be correct, but rather to not find any surprises. Under MSVC it shouldn't be much of an issue though, as this compiler doesn't handle strict aliasing at all.

src/inc/til/rect.h

DHowett · 2022-05-13T18:44:05Z

src/inc/til/rect.h

+            return std::launder(reinterpret_cast<RECT*>(this));
+        }
+
+        // til::rect and POINT[2] have the exact same layout at the time of writing,


now this is horrifying.

can you static_assert that they have the same layout somehow

Yeah I have!

src/til/ut_til/PointTests.cpp

Co-authored-by: Dustin L. Howett <[email protected]>

ghost · 2022-05-16T22:16:37Z

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

carlos-zamora · 2022-05-16T23:12:17Z

src/inc/til/bit.h

@@ -5,10 +5,20 @@

 namespace til
 {
+    // bit_cast is a backport of the STL's std::bit_cast to C++17.
    template<class To, class From, std::enable_if_t<std::conjunction_v<std::bool_constant<sizeof(To) == sizeof(From)>, std::is_trivially_copyable<To>, std::is_trivially_copyable<From>>, int> = 0>
    [[nodiscard]] constexpr To bit_cast(const From& _Val) noexcept
    {
        // TODO: Replace til::bit_cast and __builtin_bit_cast with std::bit_cast


(unrelated) oh man, there's a bare TODO here haha.

Various improvements for til::hash/point/size/rect

804559b

DHowett reviewed May 13, 2022

View reviewed changes

lhecker added 2 commits May 13, 2022 02:58

Fix AuditMode checks

a5d110a

Fixed wording

256ab9d

lhecker added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label May 13, 2022

DHowett reviewed May 13, 2022

View reviewed changes

src/inc/til/rect.h Outdated Show resolved Hide resolved

DHowett reviewed May 13, 2022

View reviewed changes

src/til/ut_til/PointTests.cpp Show resolved Hide resolved

DHowett approved these changes May 13, 2022

View reviewed changes

Address feedback

d23506b

Co-authored-by: Dustin L. Howett <[email protected]>

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 16, 2022

carlos-zamora approved these changes May 16, 2022

View reviewed changes

ghost merged commit df627c2 into main May 16, 2022

ghost deleted the dev/lhecker/4015-split2 branch May 16, 2022 23:28

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various improvements for til::hash/point/size/rect #13093

Various improvements for til::hash/point/size/rect #13093

lhecker commented May 13, 2022 •

edited

Loading

DHowett May 13, 2022

lhecker May 13, 2022

DHowett May 13, 2022

lhecker May 13, 2022

DHowett May 13, 2022

lhecker May 13, 2022 •

edited

Loading

DHowett May 13, 2022

lhecker May 13, 2022

ghost commented May 16, 2022

carlos-zamora May 16, 2022

Various improvements for til::hash/point/size/rect #13093

Various improvements for til::hash/point/size/rect #13093

Conversation

lhecker commented May 13, 2022 • edited Loading

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 16, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Choose a reason for hiding this comment

lhecker commented May 13, 2022 •

edited

Loading

lhecker May 13, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.