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

Address error C2512 when compiling with Visual Studio #676

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Aug 5, 2019

Please fill in the issue number this pull request is fixing:

Fixes #670

Changes proposed in this pull request:

  • Error is avoided by explicitly passing the default argument to the constructor, i.e. UnitSytem U({}); instead of UnitSystem U;.
  • The issue appears to be that Visual Studio erroneously doesn't recognize the optional argument. See Visual Studio: Compiler Error C2512

@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #676 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #676      +/-   ##
==========================================
+ Coverage   70.62%   70.62%   +<.01%     
==========================================
  Files         372      372              
  Lines       43564    43565       +1     
==========================================
+ Hits        30768    30769       +1     
  Misses      12796    12796
Impacted Files Coverage Δ
include/cantera/base/Units.h 100% <100%> (ø) ⬆️
src/transport/GasTransport.cpp 90.58% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc8b4be...01fcfc6. Read the comment docs.

@ischoegl ischoegl force-pushed the fix-C2512-visual-studio branch 2 times, most recently from a697bee to 7c61d9c Compare August 5, 2019 19:34
@speth
Copy link
Member

speth commented Aug 5, 2019

I agree that the error seems to be in VS2019 (especially as earlier versions mostly work, although there was a related issue in VS2017 that was fixed in 8502d18).

I'm wondering whether the better fix here is to explicitly implement a default constructor and remove the default argument from the existing constructor, with a note that this is done as a workaround for an issue with VS2019. I suggest this both because for any future use of the UnitSystem class, (a) the appropriate fix is not immediately obvious due to the error message itself being incorrect, since the class does have a valid default constructor, and (b) the declaration that should work (UnitSystem U;) will work fine on all other compilers/platforms and only be discovered when tested specifically with VS2019 which means that this same issue could easily pop up again.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 5, 2019

Valid point, especially as a permanent fix is trivial. Done.

Using a default value, VS2019 (VC 14.1) complains about a missing default constructor for UnitSystem.
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Good use of a C++11 delegating constructor.

@speth speth merged commit bbdc790 into Cantera:master Aug 6, 2019
@ischoegl ischoegl deleted the fix-C2512-visual-studio branch August 10, 2019 14:21
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.

scons test fails for Visual Studio 2019
2 participants