Skip to content
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

Silence gcc new/delete warnings for texturesys #3944

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

Shootfast
Copy link
Contributor

Description

Disable GCC warnings around mismatched new/delete for shared_texturesys creation

Tests

Checklist:

  • I have read the contribution guidelines.
  • If this is more extensive than a small change to existing code, I
    have previously submitted a Contributor License Agreement
    (individual, and if there is any way my
    employers might think my programming belongs to them, then also
    corporate).
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

@Shootfast Shootfast force-pushed the silence_gcc branch 4 times, most recently from 60ae11b to 9dd8adc Compare August 17, 2023 14:21
Comment on lines 87 to 90
#pragma GCC diagnostic push
#if __GNUC__ > 10
# pragma GCC diagnostic ignored "-Wmismatched-new-delete"
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have macros defined in platform.h for these things. In the parlance we use through the rest of the code base, this would be

OIIO_PRAGMA_WARNING_PUSH
#if OIIO_GNUC_VERSION > 100000
    OIIO_GCC_ONLY_PRAGMA(diagnostic ignored "-Wmismatched-new-delete")
#endif
...
OIIO_PRAGMA_WARNING_POP

I admit that for this small example, this is not especially any more clear than the raw pragmas you used. But the point is that it'll always work properly, no future non-gcc compiler will complain about those GCC pragma (which I am kinda superstitious about, even though CI shows it appears to be fine on the compilers we test).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I've updated the pull request to use the existing macros.
I also edited the comment as this now only applies to GCC (as your macros explicitly single out that compiler). Usually clang respects gcc pragmas, but I don't have a clang toolchain easily available, and I guess the existing CI builders haven't complained so far

Signed-off-by: Larry Gritz <[email protected]>
@lgritz
Copy link
Collaborator

lgritz commented Aug 21, 2023

I took the liberty of pushing a fix to the clang-format CI complaints. If this passes CI, I will merge.

@Shootfast
Copy link
Contributor Author

One day I will remember to run the clang-format pass first 😅

@lgritz
Copy link
Collaborator

lgritz commented Aug 21, 2023

No prob. It was tricky in this case because I guess clang-format is ok with the #if and #pragma being in the first column, so it passed clang-format your first time through. But after changing to the macros as I suggested, clang-format thought it should be indented like regular code, and started complaining.

One day I (or hopefully somebody else?) will figure out how to do a special GitHub action that will take a failed CI clang-format test and automatically turn it into an appended comment on the PR with the suggested diff of changes that can be merged in a single click. Actually, it appears there are a number of such implementations in the GitHub Action marketplace, the main task is probably to figure out which one suits our needs.

@lgritz lgritz merged commit 9909b4b into AcademySoftwareFoundation:master Aug 21, 2023
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Aug 25, 2023
…eFoundation#3944)

Disable GCC warnings around mismatched new/delete for shared_texturesys
creation


Signed-off-by: Shootfast <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
Co-authored-by: Larry Gritz <[email protected]>
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.

2 participants