-
Notifications
You must be signed in to change notification settings - Fork 242
Cherry picked include fixes #1469
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
Cherry picked include fixes #1469
Conversation
Ooo, I think some of these extra header includes shines a light on some bugs :-) So adding the vec3.h header to math.h just hides the bug of vec3 return types being used in stead of vec2: template<typename T>
t_vec3<T> operator*(const t_mat2<T>& lhs, const t_vec2<T>& rhs)
{
return t_vec3<T>((lhs[0][0] * rhs[0] + lhs[1][0] * rhs[1]),
(lhs[0][1] * rhs[0] + lhs[1][1] * rhs[1]));
}
template<typename T>
t_vec3<T> operator*(const t_vec2<T>& lhs, const t_mat2<T>& rhs)
{
return t_vec3<T>(lhs[0] * rhs[0][0] + lhs[1] * rhs[0][1],
lhs[0] * rhs[1][0] + lhs[1] * rhs[1][1]);
} I will do a full code review of the changes and see if I can spot other cases like this. |
@@ -12,12 +12,16 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI | |||
|
|||
</editor-fold> */ | |||
|
|||
#ifdef TRACY_ENABLE |
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 header will only ever be included by applications that use Tracy so there is do point add TRACY_ENABLE, adding it inappropriate like this would break the build of their application if they actually used TRACY_ENABLE.
I think this is an example of trying to quieten false positives and breaking things in the process.
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 other parts of this file that use Tracy are already fenced off behind the same preprocessor check, so it's always required this define to be set to work, and never done anything productive when it's unset. I hadn't seen that Tracy's own headers disable most of their declarations when this is unset, and assumed it came from VSG.
It sounds like this file needs an exclusion if we're going to have a check in CI.
include/vsg/maths/mat2.h
Outdated
@@ -13,6 +13,7 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI | |||
</editor-fold> */ | |||
|
|||
#include <vsg/maths/vec2.h> | |||
#include <vsg/maths/vec3.h> |
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 one hides a bug! Now I've spotted the problem I've fixed it with commit: fc3fd40 which makes this include inappropriate.
Is there a github way of removing files from a PR? There are changes which I'm happy with an want to merge and there others that are inappropriate that shouldn't be merged (TracyIntrumentation.h and mat2.h). |
If it's the PR author or if you have permission to write to the PR branch, just make the change locally and push to the remote. That will update the PR. Otherwise I'd manipulate the PR as a branch locally, merge it master, then close the PR by hand:
|
I know how to make changes through merging and branch then making changes, but was curious if I could just select what files to merge and what ones to merge based on the PR directly via the github web interface. |
cbe49c2
to
214bab1
Compare
I've made the relevant changes. |
@@ -12,6 +12,7 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI | |||
|
|||
</editor-fold> */ | |||
|
|||
#include <vsg/core/Inherit.h> | |||
#include <vsg/core/Object.h> |
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 Object.h include is now redundant as it Inherit.h includes it. I'll merge this PR and remove remove the Object.h header.
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 have merged this PR and removed the no longer required include: 51cff49
This should just be the uncontroversial parts of #1467