keszocze / aarith

An arbitrary precision number library for C++
Apache License 2.0
5 stars 5 forks source link

`std::uniform_int_distribution<>` is not defined for `unsigned char` #2

Open mbeutel opened 2 years ago

mbeutel commented 2 years ago

Several unit tests make use of std::uniform_int_distribution<>, which has the following requirement for its type parameter:

IntType – The result type generated by the generator. The effect is undefined if this is not one of short, int, long, long long, unsigned short, unsigned int, unsigned long, or unsigned long long.

In aarith's test suite there are several instances where std::uniform_int_distribution<> is indirectly instantiated with std::uint8_t, which is unsigned char on most platforms, and hence not supported as a parameter type for std::uniform_int_distribution<>:

Although uint16_t, uint32_t, uint64_t are usually identified with one of unsigned short, unsigned int, unsigned long, or unsigned long long, this is not guaranteed by the standard. It would probably be better not to use sized integer types for std::uniform_int_distribution<> and to stick with the explicitly supported types instead.

Repro: try to build aarith on Windows with MSVC or Clang. Microsoft's STL has a static_assert() enforcing above requirement.

keszocze commented 2 years ago

First of all a warning: I do not have a Windows machine and, hence, will not be able to reproduce any of this.

This issue mainly affects the means of automatically generating values for the test cases (we don't use the generation of random word_arrays etc anywhere right now). Restricting ourselves to the supported numbers will only reduce the number of test cases.

I will

Please note that this is not a pressing issue on my end and, hence, you might have to wait a bit for a first draft of the solution. I' ll send you a ping if I pushed a corresponding branch.

I just have a question regarding the fixed-width-integers. CPPReference suggests that since C++11 they are part of the standard, i.e., if the cstdint header is present, types such as uint8_t behave as expected. Do I misunderstand this?

mbeutel commented 2 years ago

First of all a warning: I do not have a Windows machine and, hence, will not be able to reproduce any of this.

(Side note: I can recommend the free offerings of Continuous Integration for open source projects, e.g. Github Actions, Azure Pipelines, Appveyor, Travis CI. All of these vendors support Windows, Linux, and MacOS. I don't have a Mac, but using the free CI I can still build and test several projects of mine with AppleClang on MacOS.)

Please note that this is not a pressing issue on my end

Not on mine, either. I just wanted to document it.

I just have a question regarding the fixed-width-integers. CPPReference suggests that since C++11 they are part of the standard, i.e., if the cstdint header is present, types such as uint8_t behave as expected. Do I misunderstand this?

The header should always be present in a conforming implementation, but according to the documentation some of its contents are optional, in particular std::intN_t and std::uintN_t must be provided "if and only if the implementation directly supports the type." So it's entirely possible that, say, std::int8_t and std::uint8_t are not defined but the others are.

I can't think of a good reason why std::uniform_int_distribution<> couldn't support char, signed char, or unsigned char; but that's just the way it is. So despite the existence of std::uint8_t (typically an alias for unsigned char), it unfortunately can't be used in std::uniform_int_distribution<>.

mbeutel commented 2 years ago

Following up on my side note, I'd first like to clarify that I don't mean to express any kind of expectation here. I'm thankful that your library exists and that you made it open to the public, and I'm glad to take it as is. I like to build my code with MSVC or Clang on Windows because these are the tools I happen to be most comfortable with, but I realize this makes me more prone to running into corner case issues than if I had just gone with GCC on Linux. This is completely at my own risk.

Nevertheless, even if you have no intention of supporting Windows, I can recommend setting up a Windows build in CI. Different compilers and standard library implementations have different levels of scrutiny for different things, and you often get warnings or errors on one platform for issues that go unnoticed on other platforms. I found building and testing my code on different platforms very helpful in unearthing bugs and inadvertent dependence on platform-specific behavior.

keszocze commented 2 years ago

We do have an internal CI building using clang and g++ and we did have a, somewhat, working CI on github. I opened a separate issue on that (see #6). There is absolutely no decision not to support Windows, I simply have no copy of it available and have to rely on github CI for this. The general idea is to be as standard conform as possible but not to entirely throw away everything. A short google search did not bring up any system that does not have uint8_t available so I will, most likely, not drop it (except from the random number generation)

If I run into trouble I will ask you about the CI on Windows process.