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

Build fixups for Visual Studio 2015 #95

Merged

Conversation

nigels-com
Copy link
Contributor

In the process of migrating from ilmbase we came across some compiler errors for Visual Studio on Windows.

https://ci.appveyor.com/project/nigels-com/displaz/builds/37721083

...
    ImathMatrixAlgo.cpp
c:\projects\displaz\build_external\imath-prefix\src\imath\src\imath\ImathVec.h(1210): error C2476: 'constexpr' constructor does not initialize all members [C:\projects\displaz\build_external\Imath-prefix\src\Imath-build\src\Imath\Imath.vcxproj] [C:\projects\displaz\build_external\Imath.vcxproj]
    c:\projects\displaz\build_external\imath-prefix\src\imath\src\imath\ImathVec.h(266): note: 'Imath_3_0::Vec3<float>::x' was not initialized by the constructor
    c:\projects\displaz\build_external\imath-prefix\src\imath\src\imath\ImathVec.h(266): note: 'Imath_3_0::Vec3<float>::y' was not initialized by the constructor
    c:\projects\displaz\build_external\imath-prefix\src\imath\src\imath\ImathVec.h(266): note: 'Imath_3_0::Vec3<float>::z' was not initialized by the constructor
    c:\projects\displaz\build_external\imath-prefix\src\imath\src\imath\ImathVec.h(1211): note: while compiling class template member function 'Imath_3_0::Vec3<float>::Vec3(void) noexcept'
    C:\projects\displaz\build_external\Imath-prefix\src\Imath\src\Imath\ImathMatrixAlgo.cpp(1190): note: see reference to function template instantiation 'Imath_3_0::Vec3<float>::Vec3(void) noexcept' being compiled
    C:\projects\displaz\build_external\Imath-prefix\src\Imath\src\Imath\ImathMatrixAlgo.cpp(243): note: see reference to class template instantiation 'Imath_3_0::Vec3<float>' being compiled
    C:\projects\displaz\build_external\Imath-prefix\src\Imath\src\Imath\ImathMatrixAlgo.cpp(255): note: see reference to function template instantiation 'Imath_3_0::M44d Imath_3_0::procrustesRotationAndTranslation<float>(const Imath_3_0::V3f *,const Imath_3_0::V3f *,const std::size_t,const bool)' being compiled
c:\projects\displaz\build_external\imath-prefix\src\imath\src\imath\ImathVec.h(1683): error C2476: 'constexpr' constructor does not initialize all members [C:\projects\displaz\build_external\Imath-prefix\src\Imath-build\src\Imath\Imath.vcxproj] [C:\projects\displaz\build_external\Imath.vcxproj]
...

This change works for both Visual Studio 2015 and 2017:
https://ci.appveyor.com/project/nigels-com/imath/builds/37723230

As an aside, I noticed that Vec2 didn't seem to have the constexpr there for Vec3 and Vec4.

Signed-off-by: Nigel Stewart <nigels@nigels.com>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 11, 2021

CLA Signed

The committers are authorized under a signed CLA.

@nigels-com
Copy link
Contributor Author

In relation to: c42f/displaz#197

@meshula
Copy link
Contributor

meshula commented Feb 12, 2021

Perhaps an alternative fix is to add initializers for x, y, z? We're working to constexpr as much as possible.

@nigels-com
Copy link
Contributor Author

For some further data-points, I built mainline Imath for Visual Studio 2013, 2015, 2017 and 2019
https://ci.appveyor.com/project/nigels-com/imath/builds/37740812

To summarise:

  • 2013 does not support C++11, failed to build
  • 2015 errors out about constexpr as above
  • 2017 succeeds
  • 2019 succeeds

Perhaps then initializers could be #ifdeffed for Visual Studio 14 2015, as a workaround without affecting runtimes elsewhere?

@nigels-com nigels-com changed the title Build fixups for Visual Studio 2015 and 2017 Build fixups for Visual Studio 2015 Feb 12, 2021
@meshula
Copy link
Contributor

meshula commented Feb 12, 2021

The historic argument for not having those fields initialized was that in the days of VS200x, and gcc 2 and 3, writing code like this:

Imath::V3f x = Imath::V3f(1,2,3);

would generate extra instructions; it would initialize x with (0,0,0), then it would copy over those values. In performance bound loops, the extra zeroing added measurable slow down.

Nowadays, I'm sure that the emitted code would be to initialize x directly with (1,2,3). I would expect that VS2015, being a much improved compiler over any of VS200x, also does the minimum work.

I'd like to solicit input from those working in Imath recently, but my suggestion is that we just add zero initializers.

@lgritz
Copy link
Contributor

lgritz commented Feb 12, 2021

I think we need to also be super careful about idioms like

std::vector<Vec3> lotsovecs(1000000);

that would incur traversing the whole array initializing, perhaps needlessly.

@meshula
Copy link
Contributor

meshula commented Feb 12, 2021

@lgritz
Copy link
Contributor

lgritz commented Feb 13, 2021

I think this patch is correct -- we should not have marked constexpr for constructors that leave fields uninitialized.

I do also notice that the constructor from a single scalar,

IMATH_HOSTDEVICE IMATH_CONSTEXPR14 explicit Vec3 (T a) noexcept;

can be constexpr rather than IMATH_CONSTEXPR14, if we change its definition from

template <class T> IMATH_CONSTEXPR14 inline Vec3<T>::Vec3 (T a) noexcept
{
    x = y = z = a;
}

to

template <class T> constexpr inline Vec3<T>::Vec3 (T a) noexcept
    : x(a), y(a), z(a)
{
}

and similarly, the Vec3 constructors from (T a, T b, T c) and const Vec3& can also be C++11 constexpr by using the member initializer syntax rather than multiple assignment statements.

@cary-ilm
Copy link
Member

I took Larry's suggestion and constexpr-ed the Vec2, Vec3, and Vec4 constructors by using member initializers, see #98.

Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

LGTM

@cary-ilm cary-ilm merged commit 03b5028 into AcademySoftwareFoundation:master Feb 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants