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

Avoid MSVC compiler bug. #728

Merged
merged 1 commit into from Apr 5, 2023
Merged

Avoid MSVC compiler bug. #728

merged 1 commit into from Apr 5, 2023

Conversation

1uc
Copy link
Collaborator

@1uc 1uc commented Apr 4, 2023

The purpose of this MR is to get older versions of MSVC to compile HighFive. The hope is it'll reproduce the issue in #724. This fixes required are also included in this PR.

@1uc
Copy link
Collaborator Author

1uc commented Apr 4, 2023

We can compare two versions: v141 and v143. They report the same version of visual studio:

-- Building for: Visual Studio 17 2022

but different compiler versions:

-- The CXX compiler identification is MSVC 19.16.27049.0
-- The CXX compiler identification is MSVC 19.34.31943.0

Checking: https://blog.knatten.org/2022/08/26/microsoft-c-versions-explained/
Suggests that those might be the compilers corresponding to the toolsets 141 and 143, respectively.

Comparing to path information found in Cantera CI we see:

Compiling with MSVC version 14.3
Compiling with MSVC toolset 14.1

https://github.com/Cantera/cantera/actions/runs/4588940048/jobs/8103492434#step:5:12

Which is probably consistent since the knatten's blog suggests that Visual Studio 2022 has MSVC versions 14.30 or 14.31.

@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #728 (053ea16) into master (3a32f30) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #728   +/-   ##
=======================================
  Coverage   82.43%   82.43%           
=======================================
  Files          67       67           
  Lines        4514     4514           
=======================================
  Hits         3721     3721           
  Misses        793      793           
Impacted Files Coverage Δ
include/highfive/H5Attribute.hpp 75.00% <ø> (ø)
include/highfive/H5File.hpp 100.00% <ø> (ø)
include/highfive/H5Group.hpp 91.66% <ø> (ø)
include/highfive/H5Object.hpp 100.00% <ø> (ø)
include/highfive/H5Utility.hpp 78.04% <ø> (ø)
include/highfive/bits/H5Utils.hpp 86.36% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@1uc 1uc force-pushed the 1uc/add-older-msvc branch 2 times, most recently from 8ec15aa to 4aec900 Compare April 5, 2023 08:07
@1uc
Copy link
Collaborator Author

1uc commented Apr 5, 2023

Should be merged after #729.

This commit reintroduces the friend decrations removed in `20be06f` for
MSVC. Certain versions of MSVC (toolset 141, possibly others) fail to
compile otherwise.

Furthermore, this commit adds CI to compile HighFive on a wider range of
MSVC compilers.
@1uc 1uc marked this pull request as ready for review April 5, 2023 08:47
@1uc 1uc changed the title Try to get older versions of MSVC into CI. Avoid MSVC compiler bug. Apr 5, 2023
@1uc 1uc mentioned this pull request Apr 5, 2023
@1uc 1uc requested review from alkino, pramodk and matz-e April 5, 2023 11:42
Copy link
Contributor

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

LGTM

@1uc 1uc merged commit 77a20c6 into master Apr 5, 2023
29 checks passed
@1uc 1uc deleted the 1uc/add-older-msvc branch April 5, 2023 17:46
@1uc 1uc mentioned this pull request Apr 5, 2023
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.

None yet

3 participants