Use type inference throughout the project #12975

lhecker · 2022-04-24T21:08:16Z

#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 ✅

lhecker · 2022-04-24T21:10:32Z

.clang-format

@@ -11,7 +11,7 @@ AlignOperands: true
 AlignTrailingComments: false
 AllowAllParametersOfDeclarationOnNextLine: false
 AllowShortBlocksOnASingleLine: Never
-AllowShortFunctionsOnASingleLine: Inline
+AllowShortFunctionsOnASingleLine: All


If you load the full diff on GitHub your browser might die. I recommend reviewing it offline.

Curiously there's no difference between Inline and All in clang-format 13 (which is what we currently use), but there is a difference in the upcoming 14! This change fixes the upcoming incompatibility ahead of time (and was needed in order to run clang-format 14 on this project).

zadjii-msft · 2022-04-25T09:41:33Z

If you load the full diff on GitHub your browser might die. I recommend reviewing it offline.

For internal folks: Install CodeFlow, then run:

\\codeflow\public\cf.cmd openGitHubPr -webUrl https://github.com/microsoft/terminal/pull/12975

zadjii-msft · 2022-04-25T10:01:14Z

src/cascadia/WindowsTerminal/IslandWindow.cpp

@@ -207,7 +207,7 @@ LRESULT IslandWindow::_OnSizing(const WPARAM wParam, const LPARAM lParam)
    // bad parameters, which we won't have, so no big deal.
    LOG_IF_FAILED(GetDpiForMonitor(hmon, MDT_EFFECTIVE_DPI, &dpix, &dpiy));

-    const long minWidthScaled = minimumWidth * dpix / USER_DEFAULT_SCREEN_DPI;
+    const LONG minWidthScaled = minimumWidth * dpix / USER_DEFAULT_SCREEN_DPI;


Weird, this one didn't turn into an auto?

No, the expression evaluates to an UINT actually, because dpix is an UINT.
The old code unsafely casts it to a long, but I think in practice that's just fine. It's unlikely there to be over 2B pixels wide screens after all. 😅

I'll revert this change to long to reduce the diff by 1.

zadjii-msft · 2022-04-25T10:02:10Z

src/cascadia/WindowsTerminal/IslandWindow.cpp

@@ -231,12 +231,12 @@ LRESULT IslandWindow::_OnSizing(const WPARAM wParam, const LPARAM lParam)
        // If user has dragged anything but the top or bottom border (so e.g. left border,
        // top-right corner etc.), then this means that the width has changed. We thus ask to
        // adjust this new width so that terminal(s) is/are aligned to their character grid(s).
-        clientWidth = gsl::narrow_cast<int>(_pfnSnapDimensionCallback(true, gsl::narrow_cast<float>(clientWidth)));
+        clientWidth = static_cast<decltype(clientWidth)>(_pfnSnapDimensionCallback(true, static_cast<float>(clientWidth)));


static_cast<decltype(clientWidth)>

Huh, this one seems weird - just getting rid of the int here? Seems like this isn't really adding anything

You're right, I'll revert it to use gsl::narrow_cast, but I'll keep the decltype(clientWidth), because that adds something. clientWidth is a long and not a int after all.

zadjii-msft · 2022-04-25T10:10:23Z

src/cascadia/TerminalApp/CommandPalette.cpp

@@ -804,8 +804,8 @@ namespace winrt::TerminalApp::implementation
    // - <unused>
    // Return Value:
    // - <none>
-    void CommandPalette::_filterTextChanged(IInspectable const& /*sender*/,
-                                            Windows::UI::Xaml::RoutedEventArgs const& /*args*/)
+    void CommandPalette::_filterTextChanged(const IInspectable& /*sender*/,


const IInspectable&

Random place to ask this, but will the const T& thing be enforced in the formatter? IIRC the T const& style comes from the auto-generated cppwinrt code, so it's entirelly likely that this leaks back into the project in the future.

In the future we can enforce this with clang-format 14. Until then I'm fine with fixing the ~770 places where we use non-standard ordering for those. Most of these are in console code actually IIRC.

zadjii-msft · 2022-04-25T11:10:44Z

src/host/settings.cpp

@@ -283,7 +283,7 @@ void Settings::Validate()
    {
        // TODO: FIX
        //// Get the font that we're going to use to convert pixels to characters.
-        //DWORD const dwFontIndexWant = FindCreateFont(_uFontFamily,
+        // const auto dwFontIndexWant = FindCreateFont(_uFontFamily,


const auto dwFontIndexWant

huh.

Is this change wrong?

ah no, I was merely observing that this refactor also applied to commented out code. There's a few other places too. Kinda neat IMO.

I had to touch up a couple hundred lines of code because clang-format / Resharper didn't get it. In some places for instance auto const wasn't replaced with const auto.
I've used \b(\w+) const (\w+) = to find code like that and fixed that code by hand, including this comment.

zadjii-msft

Okay, read all 435 files.

way to pad your contribution stats 😛
doesn't look like there's malicious-shit.exe hiding in here
Any way to enforce this in the future? Presumably audit mode could catch it (for the subset of projects that's enabled for)
- (I don't care if there's not, just curious)

zadjii-msft · 2022-04-25T11:40:24Z

@msftbot merge this in like 4 hours

ghost · 2022-04-25T11:40:28Z

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 won't merge this pull request until after the UTC date Mon, 25 Apr 2022 15:40:26 GMT, which is in 4 hours

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".

zadjii-msft · 2022-04-25T11:41:19Z

Marking for automerge cause this touches bloody everything and I'm not gonna force anyone else to sit here and read these, but leaving a window to be overruled.

ghost · 2022-05-24T17:03:33Z

🎉Windows Terminal Preview v1.14.143 has been released which incorporates this pull request.:tada:

Handy links:

PR #13665 reverted this part of paint.cpp to a time before #13097 and #12975. This commit restores those changes.

Standardize pointer/reference alignment

5b0a42f

lhecker commented Apr 24, 2022

View reviewed changes

lhecker changed the title ~~Standardize pointer/reference alignment~~ Use type inference through the project Apr 24, 2022

Use type inference wherever possible

8068696

lhecker force-pushed the dev/lhecker/8000-part0 branch from 7f3d9a6 to 8068696 Compare April 24, 2022 21:44

lhecker changed the title ~~Use type inference through the project~~ Use type inference throughout the project Apr 24, 2022

lhecker marked this pull request as ready for review April 24, 2022 23:11

zadjii-msft reviewed Apr 25, 2022

View reviewed changes

Address feedback

a467640

zadjii-msft approved these changes Apr 25, 2022

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 25, 2022

zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Apr 25, 2022

ghost merged commit 57c3953 into main Apr 25, 2022

ghost deleted the dev/lhecker/8000-part0 branch April 25, 2022 15:40

DHowett mentioned this pull request Aug 16, 2022

Restore type inference and integer casting changes in VtEngine #13760

Merged

ghost pushed a commit that referenced this pull request Aug 17, 2022

Restore type inference and integer casting changes in VtEngine (#13760)

8ba43f9

PR #13665 reverted this part of paint.cpp to a time before #13097 and #12975. This commit restores those changes.

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use type inference throughout the project #12975

Use type inference throughout the project #12975

lhecker commented Apr 24, 2022 •

edited

Loading

lhecker Apr 24, 2022

zadjii-msft commented Apr 25, 2022

zadjii-msft Apr 25, 2022

lhecker Apr 25, 2022

zadjii-msft Apr 25, 2022

lhecker Apr 25, 2022

zadjii-msft Apr 25, 2022

lhecker Apr 25, 2022 •

edited

Loading

zadjii-msft Apr 25, 2022

zadjii-msft Apr 25, 2022

lhecker Apr 25, 2022

zadjii-msft Apr 25, 2022

lhecker Apr 25, 2022

zadjii-msft left a comment

zadjii-msft commented Apr 25, 2022

ghost commented Apr 25, 2022

zadjii-msft commented Apr 25, 2022

ghost commented May 24, 2022

Use type inference throughout the project #12975

Use type inference throughout the project #12975

Conversation

lhecker commented Apr 24, 2022 • edited Loading

References

Validation Steps Performed

Choose a reason for hiding this comment

zadjii-msft commented Apr 25, 2022

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 Apr 25, 2022 • edited Loading

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

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft commented Apr 25, 2022

ghost commented Apr 25, 2022

zadjii-msft commented Apr 25, 2022

ghost commented May 24, 2022

lhecker commented Apr 24, 2022 •

edited

Loading

lhecker Apr 25, 2022 •

edited

Loading