Replace random_shuffle by shuffle

[SunBlack]

Changes are done by run-clang-tidy -header-filter='.*' -checks='-*,modernize-replace-random-shuffle' -fix

[Morwenn]

Beware of std::random_device: while it mostly works fine, it actually returns a deterministic sequence (always the same one even) on MinGW-w64, and it most likely won't be fixed before GCC 10.

[SunBlack]

Just to mention: This Pr is ready to merge, so after merge we can maybe check, if PCL can now compiled with C++17.

[taketwo]

@Morwenn thanks for the tip. Do I understand you right that this affects only MinGW? I haven't seen any support questions about this platform for ages, so I assume that it's not popular among PCL users. With this in mind I think making PCL c++17 compatible is more important.

[Morwenn]

@taketwo Yes, it only affects MinGW derivatives (including MinGW-w64). I use it for personal projects but I can't tell how much it is used with PCL since I'm only using PCL at work, so maybe you can safely ignore the correctness for the platform.

It could be worth adding a note in the documentation though, just mentioning that operations relying on random number are only expected to work with MinGW[-w64] when used with the version corresponding to GCC10 (speculative though, I don't know whether the fix has been merged). An explicit note is probably better than a silent error that could go unnoticed for too long :)

[taketwo]

Hm, we already have a bunch of std::random_device scattered in PCL code. Would you suggest adding notes to all affected classes, or rather some centralized note? If so, then (as a user) where would you expect to find such note?

[Morwenn]

A note in the FAQ or eventually in a new Troubleshooting or Common issues or Platform-specific issues in the wiki would be the places where I would expect to find such information.

[SunBlack]

@Morwenn: Is this bug only affecting std::random_device? Which seed generator is shuffle using?

[Morwenn]

In libstdc++ std::random_shuffle uses std::rand, which isn't the best RNG, but as far as I know it still produces random number in MinGW.

[SergioRAgostinho]

To be fair, I don't think we have critical cases which requires good quality random number generation. I.e. until proven otherwise, I would be perfectly ok in sticking with std::rand.

[Morwenn]

I don't think it's a huge problem per se, I'm just putting myself in the shoes of the person who might be bit by it someday: it's the kind of issue that can take days to understand since it's perfectly silent.

@SergioRAgostinho The issue is that std::random_shuffle (which uses std::rand) is getting removed in C++17 and standard libraries are allowed to get rid of it (even though many will probably keep it, which is also allowed), so moving to std::shuffle does make sense.

[SergioRAgostinho]

The issue is that std::random_shuffle (which uses std::rand) is getting removed in C++17 and standard libraries are allowed to get rid of it (even though many will probably keep it, which is also allowed), so moving to std::shuffle does make sense.

That part was clear from the start. What I wanted to bring to the table was discussing the consequences of replacingstd::random_device for std::rand to initialize the random engines.

[Morwenn]

Oh, my bad then, sorry for the confusion. It's most likely fine if std::random_shuffle was enough until today.

[SergioRAgostinho]

I'm merging this anyway because whatever we decide as follow-up will likely have its PR and place for discussion.

[SunBlack]

Maybe add something like this:

#ifdef __MINGW64__ && __MINGW64_VERSION_MAJOR < 10 //
unsigned int pcl_random_seed()
{
  //insert link to issue ticket here
  return std::rand();
}
#else
unsigned int pcl_random_seed()
{
  return (std::random_device()())
}
#endif

[taketwo]

Looks interesting. But we have random_device as a class member in some other cases in PCL, how would you account for those?

[SunBlack]

It is the question, if they really need always a new seed. Mostly it is enough to have one std::mt19937.