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

Suppress warning C5205 on Windows #141

Merged
merged 2 commits into from
Jan 5, 2021
Merged

Suppress warning C5205 on Windows #141

merged 2 commits into from
Jan 5, 2021

Conversation

chapulina
Copy link
Contributor

Attempting a fix for #140.

I queued a couple of test runs, but even though win-windows_local.win8 was idle (which uses VS2019 v16.8.3), they all waited to run on win-windows_nuc.win10 (which uses VS2019 v16.4.3), so I couldn't confirm that the fix works. Both CI machines seem to have the same labels, I don't know why the jobs only wanted to run on win10...

CI apart, I wonder if suppressing the new warning is the correct thing to do. It feels like VS2019 is trying to tell us something and we don't want to listen 🙉

@chapulina chapulina added the Windows Windows support label Dec 29, 2020
@chapulina chapulina requested a review from caguero December 29, 2020 19:04
@chapulina chapulina requested a review from mxgrey as a code owner December 29, 2020 19:04
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Dec 29, 2020
@chapulina
Copy link
Contributor Author

I just noticed that the job for this PR is also queuing waiting to run on win10, so I think we won't get to confirm the fix until we force it to run on win8.

@caguero
Copy link
Contributor

caguero commented Dec 29, 2020

It looks like we have our own internal test that triggers this warning and the expectation is that IGN_UTILS_WARN_IGNORE__NON_VIRTUAL_DESTRUCTOR should suppress it. I don't know why we need it but suppress it seems to be the goal here. Maybe @mxgrey can clarify?

@mjcarroll
Copy link
Contributor

@chapulina chapulina requested a review from mjcarroll January 4, 2021 19:37
@mjcarroll mjcarroll merged commit 0d0c814 into ign-cmake2 Jan 5, 2021
@mjcarroll mjcarroll deleted the chapulina/140 branch January 5, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice Windows Windows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants