-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add noexcept to all FontInfo structs #11640
Conversation
{ | ||
return _weight; | ||
} | ||
|
||
const std::wstring_view FontInfoBase::GetFaceName() const noexcept | ||
const std::wstring& FontInfoBase::GetFaceName() const noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
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 })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I thought to shorten the manual 4 X/Y comparisons into 2 struct comparisons.
src/renderer/base/fontinfo.cpp
Outdated
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 : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am 100% okay with keeping this line long, FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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. 😅
d4d8e47
to
0af3d3a
Compare
0af3d3a
to
f6b3d04
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
@@ -77,21 +60,15 @@ unsigned int FontInfoBase::GetCodePage() const | |||
// Arguments: | |||
// - buffer: the buffer into which to copy characters | |||
// - 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wchar_t (&buffer)[LF_FACESIZE]
huh, never seen that notation before
src/renderer/base/fontinfo.cpp
Outdated
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 : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprised this compiled locally! |
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. |
124aa70
to
3187767
Compare
Hello @zadjii-msft! Because this pull request has the 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 (
|
🎉 Handy links: |
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
allowing us to fill the buffer with a single memcpy.
(gsl::span's iterators inhibit any internal STL optimizations.)
All other changes are an effect of the virality of noexcept.
This is an offshoot from #11623.
Validation Steps Performed