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

Jump to bottom

Fix UIMarkdown with AuditMode #18221

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+23 −19
Open

Fix UIMarkdown with AuditMode #18221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fadf147
audit mode
PankajBhojwani Nov 20, 2024
5d06bc0
fixes
PankajBhojwani Nov 22, 2024
f3c3b2e
Merge branch 'main' of https://github.com/microsoft/terminal into dev…
PankajBhojwani Nov 22, 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
12 changes: 8 additions & 4 deletions OpenConsole.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2304,10 +2304,14 @@ Global
{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x64.Build.0 = Release|x64
{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x86.ActiveCfg = Release|Win32
{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x86.Build.0 = Release|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = Debug|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = Release|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = Release|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = Release|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.Build.0 = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = AuditMode|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.Build.0 = AuditMode|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.Build.0 = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = AuditMode|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.Build.0 = AuditMode|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|Any CPU.ActiveCfg = Debug|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|ARM64.ActiveCfg = Debug|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|ARM64.Build.0 = Debug|ARM64
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/UIMarkdown/CodeBlock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ namespace winrt::Microsoft::Terminal::UI::Markdown::implementation

struct RequestRunCommandsArgs : RequestRunCommandsArgsT<RequestRunCommandsArgs>
{
RequestRunCommandsArgs(const winrt::hstring& commandlines) :
RequestRunCommandsArgs(const winrt::hstring& commandlines) noexcept :
Commandlines{ commandlines } {};

til::property<winrt::hstring> Commandlines;
Expand Down
16 changes: 8 additions & 8 deletions src/cascadia/UIMarkdown/MarkdownToXaml.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ WUX::Documents::Paragraph MarkdownToXaml::_CurrentParagraph()
{
_lastParagraph.TextIndent(-WidthOfBulletPoint);
}
_lastParagraph.Margin(WUX::ThicknessHelper::FromLengths(IndentWidth * _indent, 0, 0, 0));
_lastParagraph.Margin(WUX::ThicknessHelper::FromLengths(gsl::narrow<double>(IndentWidth) * _indent, 0, 0, 0));
Copy link
Member

Choose a reason for hiding this comment

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

This probably works, but is most likely not what you actually intended.
What gsl::narrow does is something like that:

let narrowed = (TargetType)original_value;
let converted_back = (OriginalType)narrowed;
if (converted_back != original_value) {
    throw;
}
return narrowed;

In other words, this will throw if there was no accurate representation as a double. This is easy to show with floats, because if you run gsl::narrow<float>(0xffffffff) it'll throw as it'll actually turn into 4.2949673e9 which is nowhere near 4294967295.

The solution is to just use static_cast.

}
_root.Blocks().Append(_lastParagraph);
}
Expand Down Expand Up @@ -137,16 +137,16 @@ WUX::Documents::Run MarkdownToXaml::_NewRun()
}
return _lastRun;
}
void MarkdownToXaml::_EndRun()
void MarkdownToXaml::_EndRun() noexcept
{
_lastRun = nullptr;
}
void MarkdownToXaml::_EndSpan()
void MarkdownToXaml::_EndSpan() noexcept
{
_EndRun();
_lastSpan = nullptr;
}
void MarkdownToXaml::_EndParagraph()
void MarkdownToXaml::_EndParagraph() noexcept
{
_EndSpan();
_lastParagraph = nullptr;
Expand All @@ -162,7 +162,7 @@ WUX::Controls::TextBlock MarkdownToXaml::_makeDefaultTextBlock()

void MarkdownToXaml::_RenderNode(cmark_node* node, cmark_event_type ev_type)
{
bool entering = (ev_type == CMARK_EVENT_ENTER);
const bool entering = (ev_type == CMARK_EVENT_ENTER);

switch (cmark_node_get_type(node))
{
Expand Down Expand Up @@ -221,7 +221,7 @@ void MarkdownToXaml::_RenderNode(cmark_node* node, cmark_event_type ev_type)
if (entering)
{
_EndParagraph();
_NewRun().Text(bullets[std::clamp(_indent - _blockQuoteDepth - 1, 0, 2)]);
_NewRun().Text(gsl::at(bullets, std::clamp(_indent - _blockQuoteDepth - 1, 0, 2)));
}
break;

Expand Down Expand Up @@ -251,7 +251,7 @@ void MarkdownToXaml::_RenderNode(cmark_node* node, cmark_event_type ev_type)
const auto codeHstring{ winrt::to_hstring(cmark_node_get_literal(node)) };
// The literal for a code node always includes the trailing newline.
// Trim that off.
std::wstring_view codeView{ codeHstring.c_str(), codeHstring.size() - 1 };
const std::wstring_view codeView{ codeHstring.c_str(), codeHstring.size() - 1 };

auto codeBlock = winrt::make<winrt::Microsoft::Terminal::UI::Markdown::implementation::CodeBlock>(winrt::hstring{ codeView });
WUX::Documents::InlineUIContainer codeContainer{};
Expand Down Expand Up @@ -282,7 +282,7 @@ void MarkdownToXaml::_RenderNode(cmark_node* node, cmark_event_type ev_type)
cmark_node* parent = cmark_node_parent(node);
cmark_node* grandparent = cmark_node_parent(parent);

if (grandparent != NULL && cmark_node_get_type(grandparent))
if (grandparent != nullptr && cmark_node_get_type(grandparent))
{
tight = cmark_node_get_list_tight(grandparent);
}
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/UIMarkdown/MarkdownToXaml.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ struct MarkdownToXaml
winrt::Windows::UI::Xaml::Documents::Run _CurrentRun();
winrt::Windows::UI::Xaml::Documents::Span _CurrentSpan();
winrt::Windows::UI::Xaml::Documents::Run _NewRun();
void _EndRun();
void _EndSpan();
void _EndParagraph();
void _EndRun() noexcept;
void _EndSpan() noexcept;
void _EndParagraph() noexcept;

winrt::Windows::UI::Xaml::Controls::TextBlock _makeDefaultTextBlock();

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/inc/cppwinrt_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ public:
\
protected: \
type _##name{ __VA_ARGS__ }; \
void _set##name(const type& value) \
void _set##name(const type& value) noexcept(noexcept(_##name = value)) \
{ \
_##name = value; \
};
Expand Down
4 changes: 2 additions & 2 deletions src/inc/til/winrt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
template<typename T>
struct property
{
explicit constexpr property(auto&&... args) :
explicit constexpr property(auto&&... args) noexcept :
Copy link
Member

Choose a reason for hiding this comment

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

This should be something like noexcept(std::is_nothrow_constructible_v<T, decltype(args)...>, aka:
𓀀 𓁐 𓁛 𓁼 𓄿 𓆄 𓆑 𓆟 𓆣 𓆭 𓈝 𓊝 𓊩 𓊯𓋑 𓌪 𓌳 𓍯 𓎵

_value{ std::forward<decltype(args)>(args)... } {}

property& operator=(const property& other) = default;

T operator()() const noexcept
T operator()() const noexcept(noexcept(static_cast<T>(_value)))
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be std::is_nothrow_copy_constructible, but I think it would be even more ideal for us to just return const T& here and leave the unconditional noexcept.

{
return _value;
}
Expand Down
Loading