-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
event changes for DATAS #103405
event changes for DATAS #103405
Conversation
changes in gc.h are temporary and will be reverted. |
temp_change = 0x0020 | ||
}; | ||
|
||
bool should_change (float tcp, float* tcp_to_consider, size_t current_gc_index, |
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.
Is there a reason why we put the implementation of all these functions in the header file?
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.
just a personal choice, I found it easier to look at them side by side (so have gcpriv.h in one window and gc.cpp with the callersites in a different window) since I often look at them together. it's easier than having to scroll around in one file.
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.
LGTM - only comment I have is the same stylistic one as what @markples does re: adding another space to the TRACE_GC and SIMPLE_DPRINTF.
reverted the white space only changes. |
dec_multiple = 0x0004, | ||
fluctuation = 0x0008 | ||
}; | ||
|
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.
can remove the clutter from the code with something like
inline hc_change_freq_reason operator| (hc_change_freq_reason _a, hc_change_freq_reason _b) {
return static_cast<hc_change_freq_reason>(static_cast<int>(_a) | static_cast<int>(_b));
}
since we want to enable this, I needed to add necessary diag info in the events.
will be doing some testing later today.