Skip to content
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.

Sign up for GitHub

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

Remove dependencies on unstable MSVC/STL extensions, #18334

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Remove dependencies on unstable MSVC/STL extensions, #18334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3ddadf4
Update GSL header names, remove dependencies on unstable STL extensio…
YexuanXiao Dec 17, 2024
40e962c
Try to suppress false positive warnings.
YexuanXiao Dec 17, 2024
cf2bd10
Fix TIL's dependency on GSL.
YexuanXiao Dec 17, 2024
92eb451
Fix TIL's dependency on GSL.
YexuanXiao Dec 17, 2024
e39d5ff
Replace string with vector, not used as character nor need the null t…
YexuanXiao Dec 17, 2024
c2a4a14
Move #include <cassert> to pch.h.
YexuanXiao Dec 17, 2024
a199a35
Revert GSL header files.
YexuanXiao Dec 17, 2024
01a2470
Simplify code and reject old STL.
YexuanXiao Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/host/VtIo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,10 +567,11 @@ void VtIo::Writer::WriteUTF16(std::wstring_view str) const

// C++23's resize_and_overwrite is too valuable to not use.
// It reduce the CPU overhead by roughly half.
#if !defined(_HAS_CXX23) || !_HAS_CXX23
#if !defined(__cpp_lib_string_resize_and_overwrite) && _MSVC_STL_UPDATE >= 202111L
Copy link
Author

@YexuanXiao YexuanXiao Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using feature test macros instead of MSVC private macros; this will no longer be necessary after upgrading to C++23. The macro _MSVC_STL_UPDATE is documented.

#define resize_and_overwrite _Resize_and_overwrite
#elif !defined(__cpp_lib_string_resize_and_overwrite)
#error "rely on resize_and_overwrite"
#endif

// NOTE: Throwing inside resize_and_overwrite invokes undefined behavior.
_io->_back.resize_and_overwrite(totalUTF8Cap, [&](char* buf, const size_t) noexcept {
const auto len = WideCharToMultiByte(CP_UTF8, 0, str.data(), gsl::narrow_cast<int>(incomingUTF16Len), buf + existingUTF8Len, gsl::narrow_cast<int>(incomingUTF8Cap), nullptr, nullptr);
Expand Down
1 change: 0 additions & 1 deletion src/inc/LibraryIncludes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
// GSL
// Block GSL Multi Span include because it both has C++17 deprecated iterators
// and uses the C-namespaced "max" which conflicts with Windows definitions.
#include <gsl/gsl>
#include <gsl/gsl_util>
#include <gsl/pointers>

Expand Down
1 change: 1 addition & 0 deletions src/renderer/atlas/pch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <span>
#include <string_view>
#include <vector>
#include <cassert>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code using assert but did not include the header cassert, rely on the unstable internal include relationship of STL. I think it would be better to fix it.


#include <d2d1_3.h>
#include <d3d11_2.h>
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/gdi/gdirenderer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ namespace Microsoft::Console::Render
// It's important the pool is first so it can be given to the others on construction.
std::pmr::unsynchronized_pool_resource _pool;
std::pmr::vector<std::pmr::wstring> _polyStrings;
std::pmr::vector<std::pmr::basic_string<int>> _polyWidths;
std::pmr::vector<std::pmr::vector<int>> _polyWidths;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used as string nor need the null terminator.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this line was changed with the title of this PR. IIRC, some developers use std::basic_string<T> for non-char types to express small arrays of type T in order to make use of SSO behaviour of std::basic_string<>. The null terminator is also only guaranteed to be present when .c_str() is invoked, if I'm not mistaken (please correct me otherwise :) )

Copy link
Author

@YexuanXiao YexuanXiao Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::basic_string<int> is actually std::basic_string<int, std::char_traits<int>, std::allocator<int>>. However, the standard doesn't allow std::char_traits<int>. STL supports it because of historical mistake. When CharType is int32_t or char32_t, the STL (as well as other standard library implementations) have a very small SSO capacity of 3, making it essentially pointless. Since C++11, it is prohibited to add the null terminator when calling c_str.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thanks. 👍


std::vector<DWORD> _imageMask;

Expand Down
2 changes: 1 addition & 1 deletion src/renderer/gdi/paint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
polyString.back() &= softFontCharMask;
polyWidth.push_back(gsl::narrow<int>(cluster.GetColumns()) * coordFontSize.width);
cchCharWidths += polyWidth.back();
polyWidth.append(text.size() - 1, 0);
polyWidth.resize(polyWidth.size() + text.size() - 1);
}

// Detect and convert for raster font...
Expand Down
4 changes: 2 additions & 2 deletions src/server/WaitBlock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ class ConsoleWaitBlock
_In_ IWaitRoutine* const pWaiter);

ConsoleWaitQueue* const _pProcessQueue;
std::_List_const_iterator<std::_List_val<std::_List_simple_types<ConsoleWaitBlock*>>> _itProcessQueue;
typename std::list<ConsoleWaitBlock*>::const_iterator _itProcessQueue;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the use of unstable STL internal names.


ConsoleWaitQueue* const _pObjectQueue;
std::_List_const_iterator<std::_List_val<std::_List_simple_types<ConsoleWaitBlock*>>> _itObjectQueue;
typename std::list<ConsoleWaitBlock*>::const_iterator _itObjectQueue;

CONSOLE_API_MSG _WaitReplyMessage;

Expand Down
2 changes: 1 addition & 1 deletion src/terminal/adapter/SixelParser.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ namespace Microsoft::Console::VirtualTerminal
size_t _colorsUsed = 0;
size_t _colorsAvailable = 0;
bool _colorTableChanged = false;
IndexedPixel _foregroundPixel = 0;
IndexedPixel _foregroundPixel = {};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The struct has two members, and allowing the use of = 0 initialization might be an MSVC extension or a bug.


void _initImageBuffer();
void _resizeImageBuffer(const til::CoordType requiredHeight);
Expand Down
7 changes: 6 additions & 1 deletion src/types/colorTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,9 @@ void Utils::InitializeVT340ColorTable(const std::span<COLORREF> table) noexcept
}
}

#pragma warning(push)
#pragma warning(disable : 26497) // This is a false positive in audit mode.

// Function Description:
// - Fill color table entries from 16 to 255 with the default colors used by
// modern terminals. This includes a range of colors from a 6x6x6 color cube
Expand All @@ -255,7 +258,7 @@ void Utils::InitializeVT340ColorTable(const std::span<COLORREF> table) noexcept
// - table: a color table to be filled
// Return Value:
// - <none>
constexpr void Utils::InitializeExtendedColorTable(const std::span<COLORREF> table, const bool monochrome) noexcept
void Utils::InitializeExtendedColorTable(const std::span<COLORREF> table, const bool monochrome) noexcept
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the strange constexpr as it might cause the function definition to be discarded, leading to missing symbols.

{
if (table.size() >= 256)
{
Expand All @@ -279,6 +282,8 @@ constexpr void Utils::InitializeExtendedColorTable(const std::span<COLORREF> tab
}
}

#pragma warning(pop)

#pragma warning(push)
#pragma warning(disable : 26447) // This is a false positive.

Expand Down
2 changes: 1 addition & 1 deletion src/types/inc/colorTable.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace Microsoft::Console::Utils
void InitializeColorTable(const std::span<COLORREF> table) noexcept;
void InitializeANSIColorTable(const std::span<COLORREF> table) noexcept;
void InitializeVT340ColorTable(const std::span<COLORREF> table) noexcept;
constexpr void InitializeExtendedColorTable(const std::span<COLORREF> table, const bool monochrome = false) noexcept;
void InitializeExtendedColorTable(const std::span<COLORREF> table, const bool monochrome = false) noexcept;
std::span<const til::color> CampbellColorTable() noexcept;

std::optional<til::color> ColorFromXOrgAppColorName(const std::wstring_view wstr) noexcept;
Expand Down
Loading