Add noexcept to all FontInfo structs by lhecker · Pull Request #11640 · microsoft/terminal

lhecker · 2021-10-28T12:55:27Z

FontInfoBase and it's descendents are missing noexcept annotations, which
virally forces other code to not be noexcept as well during AuditMode checks.
Apart from adding noexcept, this commit also

Passes std::wstring_view by reference.
Pass the FillLegacyNameBuffer argument as a simple pointer-to-array,
allowing us to fill the buffer with a single memcpy.
(gsl::span's iterators inhibit any internal STL optimizations.)
Move operator== declarations inside the class to reduce code size.

All other changes are an effect of the virality of noexcept.

This is an offshoot from #11623.

Validation Steps Performed

It still compiles ✔️

lhecker · 2021-10-28T12:56:33Z

src/renderer/base/FontInfoBase.cpp

 }

-const std::wstring_view FontInfoBase::GetFaceName() const noexcept
+const std::wstring& FontInfoBase::GetFaceName() const noexcept


This change allows you to call the technically more correct and safer .c_str() on the return value instead of hoping that the string_view refers to a null terminated string. (This could be changed in the UIA and GDI renderers. I'm making direct use of this safety in the AtlasEngine.)

lhecker · 2021-10-28T12:57:19Z

src/renderer/base/FontInfoDesired.cpp

-    return WasDefaultRasterSetFromEngine() || (GetFaceName().empty() &&
-                                               ((_coordSizeDesired.X == 0 && _coordSizeDesired.Y == 0) ||
-                                                (_coordSizeDesired.X == 8 && _coordSizeDesired.Y == 12)));
+    return WasDefaultRasterSetFromEngine() || (GetFaceName().empty() && (_coordSizeDesired == COORD{ 0, 0 } || _coordSizeDesired == COORD{ 8, 12 }));


Here I thought to shorten the manual 4 X/Y comparisons into 2 struct comparisons.

lhecker · 2021-10-28T12:58:17Z

src/renderer/base/fontinfo.cpp

-                   const COORD coordSize,
-                   const unsigned int codePage,
-                   const bool fSetDefaultRasterFont /* = false */) :
+FontInfo::FontInfo(const std::wstring_view& faceName, const unsigned char family, const unsigned int weight, const COORD coordSize, const unsigned int codePage, const bool fSetDefaultRasterFont) noexcept :


BTW The reason I scram these into a single line is because

(They still fit on most monitors and...)

Because it makes copy-pasting definitions into declarations and vice versa way easier IMHO

fwiw, I hate long single lines like this because I usually have two side-by-side panes, so 80-100 columns of width is preferable to me. Like, this line is 206 columns:

which is like 75% of the width of my monitor.

IDGAF, we'll just be in a constant struggle, you and I 😝

Because it makes copy-pasting definitions into declarations and vice versa way easier IMHO

I usually use the "create definition / declaration" feature in VS ;P

I meant something like: If each definition is one line long you can use Ctrl-Alt-Up/Down to insert multiple cursors at the start of each line and for instance quickly add additional annotations at the end of each line. However if declarations are multiple lines long you need to hope the functions don’t use SAL or function pointers (for instance). (This is really just one example.)
I‘ll just revert this.

I am 100% okay with keeping this line long, FYI

@zadjii-msft If you want to you can re-review the latest 2 commits, which make this PR a bit more compact - both horizontally and diff-wise. 😅

zadjii-msft

sure

zadjii-msft · 2021-10-28T15:24:02Z

src/renderer/base/FontInfoBase.cpp

 // - size: the size of buffer
-HRESULT FontInfoBase::FillLegacyNameBuffer(gsl::span<wchar_t> buffer) const
-try
+void FontInfoBase::FillLegacyNameBuffer(wchar_t (&buffer)[LF_FACESIZE]) const noexcept


wchar_t (&buffer)[LF_FACESIZE]

huh, never seen that notation before

zadjii-msft · 2021-10-28T15:31:38Z

src/renderer/base/fontinfo.cpp

-                   const COORD coordSize,
-                   const unsigned int codePage,
-                   const bool fSetDefaultRasterFont /* = false */) :
+FontInfo::FontInfo(const std::wstring_view& faceName, const unsigned char family, const unsigned int weight, const COORD coordSize, const unsigned int codePage, const bool fSetDefaultRasterFont) noexcept :


fwiw, I hate long single lines like this because I usually have two side-by-side panes, so 80-100 columns of width is preferable to me. Like, this line is 206 columns:

which is like 75% of the width of my monitor.

IDGAF, we'll just be in a constant struggle, you and I 😝

DHowett · 2021-10-28T17:22:11Z

Surprised this compiled locally!

lhecker · 2021-10-28T17:50:39Z

I only compiled Host.EXE. I didn’t consider the possibility that the FontInfo objects are used in WT classes without being used in OpenConsole. I‘ll fix the issues soon.

ghost · 2021-10-29T14:06:30Z

Hello @zadjii-msft!

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.

ghost · 2022-02-03T18:23:39Z

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

Handy links:

lhecker added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Oct 28, 2021

lhecker commented Oct 28, 2021

View reviewed changes

lhecker force-pushed the dev/lhecker/font-info-noexcept branch from d4d8e47 to 0af3d3a Compare October 28, 2021 13:51

Add noexcept to all FontInfo structs

f6b3d04

lhecker force-pushed the dev/lhecker/font-info-noexcept branch from 0af3d3a to f6b3d04 Compare October 28, 2021 14:43

zadjii-msft approved these changes Oct 28, 2021

View reviewed changes

DHowett approved these changes Oct 28, 2021

View reviewed changes

lhecker added 2 commits October 29, 2021 02:47

Fix build issues

1ede43f

Address Mike's comments

3187767

lhecker force-pushed the dev/lhecker/font-info-noexcept branch from 124aa70 to 3187767 Compare October 29, 2021 12:11

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 29, 2021

ghost merged commit 95cc7d9 into main Oct 29, 2021

ghost deleted the dev/lhecker/font-info-noexcept branch October 29, 2021 14:08

This pull request was closed.

Conversation

lhecker commented Oct 28, 2021

Validation Steps Performed

Uh oh!

lhecker Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented Oct 28, 2021

Uh oh!

lhecker commented Oct 28, 2021

Uh oh!

ghost commented Oct 29, 2021

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.

Uh oh!

ghost commented Feb 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lhecker Oct 28, 2021 •

edited

Loading

lhecker Oct 28, 2021 •

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.