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

fix: Ignore C4668 when preprocessing via MSVC #1730

Conversation

Alexei-Barnes
Copy link
Contributor

This fixes #1725.

@sylvestre
Copy link
Collaborator

is it possible to add a test for this? thanks

@Alexei-Barnes
Copy link
Contributor Author

Absolutely, I'm on vacation right now without access to a computer I could run the tests on. Back home around the 7th of May. Happy to amend this with a test then.

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.04 ⚠️

Comparison is base (7416904) 29.44% compared to head (071d680) 29.40%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1730      +/-   ##
==========================================
- Coverage   29.44%   29.40%   -0.04%     
==========================================
  Files          49       49              
  Lines       17713    17713              
  Branches     8547     8547              
==========================================
- Hits         5215     5209       -6     
  Misses       7347     7347              
- Partials     5151     5157       +6     
Impacted Files Coverage Δ
src/compiler/msvc.rs 45.01% <ø> (ø)

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Alexei-Barnes
Copy link
Contributor Author

@sylvestre I've written a minimal integration test to verify the behaviour.

There aren't currently any unit tests for the msvc preprocessing method. I could add one if required, but it might require other changes to make the preprocessing method more testable.

@Alexei-Barnes Alexei-Barnes force-pushed the disable-c4668-when-preprocessing-with-msvc branch from 451245d to 9720864 Compare May 9, 2023 14:00
@Alexei-Barnes
Copy link
Contributor Author

Alexei-Barnes commented May 9, 2023

Updated the test just now to actually use sccache 😅
Should now actually test the intended behaviour. I also threw in a direct call to cl.exe just to show the preprocessing error in the output that we're trying to avoid. Maybe we should run this new test on unfixed code first to establish a baseline negative result?

@Alexei-Barnes Alexei-Barnes force-pushed the disable-c4668-when-preprocessing-with-msvc branch from 9720864 to df8085d Compare May 9, 2023 14:34
@Alexei-Barnes
Copy link
Contributor Author

Removed the additional call to cl that shows the 4668 warning because it terminates the build, and it isn't necessary.

@Alexei-Barnes
Copy link
Contributor Author

Oh interesting, the test failed! Will investigate tomorrow.

@Alexei-Barnes Alexei-Barnes force-pushed the disable-c4668-when-preprocessing-with-msvc branch from df8085d to 0e68e34 Compare May 10, 2023 08:29
@Alexei-Barnes
Copy link
Contributor Author

OK the test revealed that simply changing the warning to level 4 isn't enough, it must be entirely disabled. I've updated the code, test should pass in CI now.

@Alexei-Barnes
Copy link
Contributor Author

Test passing now 👍

@sylvestre sylvestre force-pushed the disable-c4668-when-preprocessing-with-msvc branch from 0e68e34 to 8cf57a8 Compare May 23, 2023 07:24
@sylvestre sylvestre force-pushed the disable-c4668-when-preprocessing-with-msvc branch from 8cf57a8 to 5310b46 Compare May 30, 2023 14:16
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.

Windows SDK Generates C4668 During Pre-processing - Confuses Sccache
3 participants