-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Remove VtApiRoutines #16954
Remove VtApiRoutines #16954
Conversation
catch (const wil::ResultException& e) | ||
{ | ||
hr = e.GetStatusCode(); | ||
} |
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 seems quite different from what we have today - does it deserve its own PR?
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.
Ironically it's what prompted me to make this change before I drifted off. I'd be happy to split this into its own PR. It doesn't change the behavior of this function (there shouldn't have been exceptions so far) nor should there be a performance difference (the catch clauses should be essentially cost free). I'm not sure whether a separate PR is better.
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.
Sad that this never did quite pan out, but we'll try something else soon I'm sure
// This is plenty of space to hold the formatted string | ||
wchar_t cmd[MAX_PATH]{}; | ||
const BOOL bInheritCursor = (dwFlags & PSEUDOCONSOLE_INHERIT_CURSOR) == PSEUDOCONSOLE_INHERIT_CURSOR; | ||
const BOOL bResizeQuirk = (dwFlags & PSEUDOCONSOLE_RESIZE_QUIRK) == PSEUDOCONSOLE_RESIZE_QUIRK; | ||
swprintf_s(cmd, | ||
MAX_PATH, | ||
pwszFormat, | ||
L"\"%s\" --headless %s%s--width %hd --height %hd --signal 0x%tx --server 0x%tx", |
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 see you changed additional format parameters here. can you quickly dip into what tx
is versus x
? we've had it the one way for like five years ;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.
x
is an unsigned int
sized parameter and so it's wrong to pass a void*
sized type. The only reason it works is because on x86 the stack word size is 8 bytes and so the alignment of those vararg parameters matches the pointer size. I'm sure it's the same on ARM. Additionally, handle values aren't usually very large.
C99 introduced 3 new prefixes:
z
:size_t
j
:intmax_t
t
:ptrdiff_t
In this case passing t
is the right choice. z
would also be okay, but only because current architectures have ptrdiff == size
.
This removes
VtApiRoutines
and the VT passthrough mode.Why? While VT passthrough mode has a clear advantage (doesn't corrupt
VT sequences) it fails to address other pain points (performance,
out-of-sync issues after resize, etc.). Alternative options are
available which have less restrictions.
Why now? It's spring! Spring cleanup!