Does Ginkgo support the MSVC 2017 compiler?

[shock-pro]

Hi Everyone,
I tried disabling OMP because of the type issue in the OMP for loop index variable.
Still, the compiler shows more than 100 errors and it has only compiled ~ 20% of the source.

IMHO it would be best if you remove VS from the supported compilers so that others do not spend time trying to build on Windows with VS.
Regards,

[upsj]

Ginkgo does support MSVC 2019 with CUDA, as you can see in our CI runs, but we should maybe bump our version requirements in case we can't manage to fix the compilation issues. Which exact MSVC version are you using?

[shock-pro]

Hi, I am using MSVC 2017, and I was trying to build Ginkgo's 1.3.0, from the syntax and type conversion issues that I run into,
I think that you may be using some flags that allow such conversions,
Because as shown below I inserted the cast (size_t) to fix that error, but then I realized there are hundreds of those mismatches.

And the compiler is not wrong because the code is using a "signed" int and it is passed as argument to an "unsigned" int,
so I think that the compiler is right in flagging it as a type mismatch.

[upsj]

Are you perhaps compiling a 32 bit binary? normally int is smaller than size_t

[shock-pro]

No I am using x64, it is clearly a signed / unsigned issue.
You can read the error, and you can find that gko::size_type is defined as size_t:
size_t is the unsigned integer type of the result of the sizeof operator (ISO C99 Section 7.17.)
If you look at threads on this subject, you'll find out that I tried modifying the def of gko::size_type as a signed int, but of course that produced a lot of other incompatibilities because in lots of places the arguments require (size_t *)

[upsj]

Then I'm pretty sure the MSVC compiler is at fault here, since row_size_limit is constexpr with a value that is representable by size_type, so the conversion is not narrowing. For more details, see https://stackoverflow.com/questions/58345244/does-the-standard-prevent-narrowing-conversion-of-literal-with-small-enough-lite. But discussions over the standard notwithstanding, the only answer I can give you is: We do no longer explicitly check for MSVC 2017 compatibility, so failures like these may pop up from time to time. We will update our documentation to make this more explicit.

[shock-pro]

There are certain things with MSVC that are not good, e.g. OMP is very very weak, MS abandon OMP support to support their own support. However, if code is passing a "signed" variable as an argument that expects an "unsigned" value, I really expect the compiler to at least warn me, in this case the compiler is flagging this as an Error, so I don't think that we can use a pragma to override this behavior. Nonetheless, I think it is a bad coding practice to pass signed variables where unsigned are expected.

[upsj]

First of all: You are completely correct that this is not the best style. We will move to all signed index types in the future, but that is only possible with the next major version without breaking interface. On the other hand, the MSVC 2017 compiler here is incorrect, as using a constexpr variable here is equivalent to writing 32 directly, which would be fine. For context, see [dcl.init]