Skip to content

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

Merged
merged 2 commits into from
May 21, 2025

Conversation

AnyOldName3
Copy link
Contributor

This should just be the uncontroversial parts of #1467

@robertosfield
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@@ -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>
Copy link
Collaborator

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.

@robertosfield
Copy link
Collaborator

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).

@timoore
Copy link
Contributor

timoore commented May 21, 2025

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:

git fetch origin pull/1489/head:include-fixes
...
git checkout master
git merge include-fixes
git push origin @

@robertosfield
Copy link
Collaborator

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.

@AnyOldName3 AnyOldName3 force-pushed the cherry-picked-include-fixes branch from cbe49c2 to 214bab1 Compare May 21, 2025 13:10
@AnyOldName3
Copy link
Contributor Author

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>
Copy link
Collaborator

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.

Copy link
Collaborator

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

@robertosfield robertosfield merged commit 7e707b1 into vsg-dev:master May 21, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants