-
Notifications
You must be signed in to change notification settings - Fork 478
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
minor fixes and additions #905
base: master
Are you sure you want to change the base?
Conversation
Hello @Slaw6820 thank you for the contribution, could you split the one commit into the separate changes? |
Sure. Does it mean that all proposed fixes are accepted? |
LTGM, but I'm not an biggest authority here :) just commits within this MR :) |
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.
Sorry for the silence -- I was on vacation last week.
Thanks for the changes. Comments inline.
Otherwise looks good. I agree it would be good to break the changes according to main modules (wrappers, retrace, etc. The main reason is that one can git bisect and revert chunks
api < other.api || | ||
core < other.core || | ||
forwardCompatible < other.forwardCompatible; | ||
return (major < other.major) || |
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.
Good catch.
There's a more compact way to achieve the same:
if (major != other.major) {
return major < other.major;
}
if (minor != other.minor) {
return minor < other.minor;
}
...
@@ -221,7 +221,16 @@ OpenGLImpl::skipDeleteObj(const trace::Call& call) | |||
map = &m_textures; | |||
|
|||
if (!strcmp(call.name(), "glDeleteFramebuffers")) | |||
map = &m_current_context->m_fbo; | |||
{ |
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.
Code formatting should be
if (condition) {
....
}
#endif | ||
|
||
|
||
#define assert(expression) \ |
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.
Why is this necessary?
During my work with apitrace I have encountered a small number of issues that were addressed with the following changes to the source code.
Please review.