Make unit.core.exe compile with MSVC

[Tradias]

• I slightly altered the logic of value_type_t it now prefers nested T::value_type over range/iterator. Not sure whether this will have a negative effect on the other modules.
• I had to add includes for frexp and exp2 to two of the tests, not sure whether it is supposed to be like that.
• include(CTest) is not necessary to run tests with ctest, it just clutters the project with unnecessary targets.
• std::inclusive_scan is in fact in the header and not

There is also an issue in https://github.com/jfalcou/tts/blob/main/include/tts/tts.hpp#L943 which can create a std::uniform_int_distribution<uint64_t> from -10 to +10 which become 18446744073709551606 to 10, which triggers an assertion in MSVC:

_STL_ASSERT(_Min0 <= _Max0, "invalid min and max arguments for uniform_int");

Resolves a part of #1236

[Tradias]

I have used the following compiler:

C:\Program Files\Microsoft Visual Studio\2022\Community>cl --version
Microsoft (R) C/C++ Optimizing Compiler Version 19.34.31933 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

and compile options:

/wd4267 /wd4244 /wd4146 /bigobj /DWIN32 /D_WINDOWS /GR /EHsc /permissive- /Zc:__cplusplus /Zc:inline /Zc:sizedDealloc /Zc:externConstexpr /Zc:lambda /Zc:throwingNew

[jfalcou]

Looks good. A few remarks:

I slightly altered the logic of value_type_t it now prefers nested T::value_type over range/iterator. Not sure whether this will have a negative effect on the other modules.

This probably have to be merged with the fix from #1458
As per the change in behavior, this will affect unit.algo. Maybe @DenisYaroshevskiy has an opinion on it.

I had to add includes for frexp and exp2 to two of the tests, not sure whether it is supposed to be like that.

This is somethign which is an oversight. There is a fix for this in an upcoming PR so this is OK for now.

include(CTest) is not necessary to run tests with ctest, it just clutters the project with unnecessary targets.

Guess it was some old habits

std::inclusive_scan is in fact in the <numeric> header and not <algorithm>

Good catch

There is also an issue in https://github.com/jfalcou/tts/blob/main/include/tts/tts.hpp#L943 which can create a std::uniform_int_distribution<uint64_t> from -10 to +10 which become 18446744073709551606 to 10, which triggers an assertion in MSVC:

Would you mind to file an issue at https://github.com/jfalcou/tts/issues ?
I need to update the TTS EVE use but I think this must be fixed anyway.

[jfalcou]

/wd4267 /wd4244 /wd4146 /bigobj /DWIN32 /D_WINDOWS /GR /EHsc /permissive- /Zc:__cplusplus /Zc:inline /Zc:sizedDealloc /Zc:externConstexpr /Zc:lambda /Zc:throwingNew

as a rather naive user of MSVC, are those flags somehow classics ? If yes, we may need to update https://github.com/jfalcou/eve/blob/main/cmake/config/compiler.cmake ?

[jfalcou]

As expected, you hit the "ambiguous value_type" eror ron clang/g++
I suggest we merge #1458 and rebase your PR ?

[jfalcou]

#1458 is merged, so I guess you can rebase

[Tradias]

Out of the compile options only /bigobj is actually needed to compile some of the tests, otherwise they fail with:

[build] C:\3YOURMIND\eve\test\unit\module\core\add.cpp : fatal error C1128: number of sections exceeded object file format limit: compile with /bigobj

The options /wd4267 /wd4244 /wd4146 disable conversion warnings and a warning about unary minus operator.

It seems that /permissive- is implied by /std:c++20 so no reason to set it explicitly in this project.

The /Zc options just enable additional C++ standard conformance behavior that is not covered by /permissive- and in rare cased helped me to write more portable code.

The options /DWIN32 /D_WINDOWS /GR /EHsc are usually set by CMake by default, basically: /GR == -frtti and /EHsc == -fexceptions.

[jfalcou]

OK so I guess you need to update the compielrs.cmake file so the appveyor CI can use those options

[jfalcou]

OK, first world problem, we hit the 1hr compile time limit on AppVeyor. I'll setup a windows machine next week so we can self-host the tests.

Alternatively, we may try to edit the appveyor.yml to compiel in Debug to save on compile time, thsi will probably requires /bigObj too

[jfalcou]

#1461 is tryign to make AppVeyor more happy

[DenisYaroshevskiy]

Looked through, looks good so far

[jfalcou]

#1461 is merged, woudl you mind rebasing if now appveyor is happy in term of compile time ?

[jfalcou]

We still hit the limitations. Trying to see what we can do before I can setup a proper self hosted windows machine

[jfalcou]

@Tradias woudl you mind modifying the appveyor.yml file so we have this:

- cmake --build . --target unit.arch.exe      --config Debug -j 2
- cmake --build . --target unit.meta.exe      --config Debug -j 2
- cmake --build . --target unit.internals.exe --config Debug -j 2
- cmake --build . --target unit.core.exe --config Debug -j 2

?

EDIT: woopsies on forgetting core

[jfalcou]

oh wait, I thought we just add -j 2 to the build, not removing the core.exe sorry

[Tradias]

In the long run you should consider to link all tests into one executable, at least for CI builds. That is significantly faster, especially on Windows (where we don't have high-speed linkers like mold)

[jfalcou]

In the long run you should consider to link all tests into one executable, at least for CI builds. That is significantly faster, especially on Windows (where we don't have high-speed linkers like mold)

Yeah that's somethign I was thinking about.
This may requires a bit of cmakery.

We could move to GA with theire 6h limit but I fear they don't have the proper version of MSVC.
I'll dig for a machine at work.

[DenisYaroshevskiy]

I mean, we tried to reduce the number of binaries, right? We even did unity builds which were supposed fto be big time faster. We got no speedups

[jfalcou]

I am gonna merge a PR that move our MSVC tests on Github Actions. THe version of the compiler is OK and we have a 6h limits.

[jfalcou]

It may be too soon, got some more error than anticipated. I'll gonna review the code and see when GA windows got updated

[Tradias]

You mean regarding https://github.com/jfalcou/eve/actions/runs/3572457982/jobs/6005370261 ? I can take a look at that too.

[jfalcou]

Yeah, I am hacking at it. Looks like same simplifcation that you already did.

[jfalcou]

And oh, the core tests have liek few errors so that's encouraging!