Updating random number generation to not overflow

[epicfarmer]

rand is a std::uniform_int_distribution<>, which defaults to int. So when it's called with 0xffffffff, that is -1.

Not sure if the right fix is to change the integer type or the initialization.

[arcondello]

Thanks for finding this!

The main thing is to keep the initialization and the class member consistent. So IMO we should either change line 215/216 to

    //! distribution over [0, 0xffffffff]
    uniform_int_distribution<std::uint32_t> rand;

and line 240 to

            rand(0, std::numeric_limits<std::uint32_t>::max()),

or update both to be int explicitly.

    //! distribution over [0, 0x7fffffff]
    uniform_int_distribution<int> rand;

            rand(0, std::numeric_limits<int>::max()),

I think the latter is probably the better approach.

There may be other places in the code that need to be updated for consistency as well.

[epicfarmer]

I've udpated the PR based on the latter. As far as

There may be other places in the code that need to be updated for consistency as well.

this is the only place where I encountered overflow that interferes with bounds checking. There are generally lots of conversions between signed/unsigned larger/smaller integer types in the codebase, but it's not obvious to me which if any are problematic.

Separately, I think the gtests are broken for me locally, and I cannot see them run on ci. I'll file a separate issue for that

[boothby]

Thanks for your attention to this; it's been a long while since I've been through this code but I'm pretty sure that the right solution is to make everything unsigned -- I don't think that I used -1 as a sentinel value anywhere, but that would be the thing to look for.

[arcondello]

I'm pretty sure that the right solution is to make everything unsigned

I'd argue consistency is all that matters. We're not short on possible values 😄
And the fact that it's strictly positive is not really a reason to use unsigned. See e.g. https://google.github.io/styleguide/cppguide.html#Integer_Types. Which is why I suggested int.

Though there is also a local consistency argument 🤷 I have no strong feelings here.

[arcondello]

Huh, unless I am missing something it seems like we only use the type of rand

minorminer/include/find_embedding/embedding_problem.hpp

Line 343 in 982644c

int randint(int a, int b) { return rand(params.rng, typename decltype(rand)::param_type(a, b)); }

still looking...

[epicfarmer]

Huh, unless I am missing something it seems like we only use the type of rand

minorminer/include/find_embedding/embedding_problem.hpp

Line 343 in 982644c

int randint(int a, int b) { return rand(params.rng, typename decltype(rand)::param_type(a, b)); }

still looking...

That feels right to me. I think if you were really drawing numbers between 0 and -1, you'd notice in the python test suite somewhere.

As for

the right solution is to make everything unsigned

I enabled -Wsign-conversion as a test, and this seems like a lot of work. It's only ~400 lines of warnings, but it seems to touch a lot of files. Here are my compilation results if you're interested:
tmp.txt

[boothby]

I enabled -Wsign-conversion as a test, and this seems like a lot of work. It's only ~400 lines of warnings, but it seems to touch a lot of files.

It is a lot of work. I see the use of int as problematic and wish that I'd gotten rid of signed types for node labels when I took over the project... but here we are. It's been on a far-back burner for years now.

[epicfarmer]

I don't mind submitting more PRs towards this goal. However, I think that:

• This is better done as a series of small PRs rather than one large one (I can start by changing this to unsigned if you want)
• Resolving cpp tests not run on ci #269 first will be key, since I don't have your architecture and will want to write tests to ensure I'm not making changes to the output.