-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 more TraceLogging to ApiDispatchers #17085
Conversation
constexpr auto min = std::numeric_limits<T>::min(); | ||
constexpr auto max = std::numeric_limits<T>::max(); | ||
#pragma warning(suppress : 4267) // '...': conversion from '...' to 'T', possible loss of data | ||
return val < min ? min : (val > max ? max : val); |
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.
If typeof(val)
is T
, these checks are UB, right? Since the compiler can probably optimize out T(x) < T(min)
as it will always be false?
should we assert that the range of T
is wider than the range of typeof(val)
?
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.
meh, it's probably fine. saturate<T>(T)
should just return t;
afterall. These checks being eliminated makes that true!
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 don't think it would be UB, it would just be always false (as you said).
I haven't really thought this function through yet, tbh. It's basically a "prototype" and that's also why I left it there and not in til.
src/server/ApiDispatchers.cpp
Outdated
"WriteConsoleInput", | ||
TraceLoggingBoolean(a->Unicode, "Unicode"), | ||
TraceLoggingBoolean(a->Append, "Append"), | ||
TraceLoggingUIntPtr(buffer.size(), "Buffer")); |
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.
we may want to call these size fields Records
or something, right? since they're not the actual buffer
TraceLoggingPackedData(&value, sizeof(COORD)), \ | ||
TraceLoggingPackedStruct(2, name), \ |
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.
what's the difference - why packed?
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.
The TraceLogging documentation (in the header file) says to use this, if one has packed struct fields, because it saves space and improves performance.
More TraceLogging = More better? I made this change as I noticed that most calls are not being logged. Even after this change some crucial information won't be logged (for instance arrays of `INPUT_RECORD`), because I couldn't come up with a clever way to do so, but I think this is better than nothing. (cherry picked from commit f49cf44) Service-Card-Id: 92374414 Service-Version: 1.19
More TraceLogging = More better? I made this change as I noticed that most calls are not being logged. Even after this change some crucial information won't be logged (for instance arrays of `INPUT_RECORD`), because I couldn't come up with a clever way to do so, but I think this is better than nothing. (cherry picked from commit f49cf44) Service-Card-Id: 92374415 Service-Version: 1.20
More TraceLogging = More better?
I made this change as I noticed that most calls are not being logged.
Even after this change some crucial information won't be logged
(for instance arrays of
INPUT_RECORD
), because I couldn't come upwith a clever way to do so, but I think this is better than nothing.