void added a comment. In D121556#3445124 <https://reviews.llvm.org/D121556#3445124>, @aaron.ballman wrote:
> In D121556#3444837 <https://reviews.llvm.org/D121556#3444837>, @void wrote: > >> In D121556#3444434 <https://reviews.llvm.org/D121556#3444434>, @MaskRay >> wrote: >> >>> In D121556#3444260 <https://reviews.llvm.org/D121556#3444260>, @void wrote: >>> >>>> In D121556#3444221 <https://reviews.llvm.org/D121556#3444221>, @MaskRay >>>> wrote: >>>> >>>>> In D121556#3444131 <https://reviews.llvm.org/D121556#3444131>, @void >>>>> wrote: >>>>> >>>>>> In D121556#3444021 <https://reviews.llvm.org/D121556#3444021>, @MaskRay >>>>>> wrote: >>>>>> >>>>>>> 7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 >>>>>>> <https://reviews.llvm.org/rG7aa8c38a9e190aea14116028c38b1d9f54cbb0b3> >>>>>>> still uses `std::shuffle`, not incorporating the `llvm::shuffle` fixes >>>>>>> I did. >>>>>> >>>>>> You said it was still failing after the std::shuffle to llvm::shuffle >>>>>> change. >>>>> >>>>> By saying it still failed, I meant there were other Windows vs >>>>> non-Windows differences, not that std::shuffle=>llvm::shuffle was an >>>>> unintended change. >>>> >>>> So does it work or not? If I change it to `llvm::shuffle`, will the tests >>>> fail or will they pass regardless of the platform? >>> >>> If you change `std::shuffle` to `llvm::shuffle` and add back tests like >>> `EXPECT_EQ(Expected, getFieldNamesFromRecord(RD));`, the test will likely >>> fail on Windows. >>> I recall that @aaron.ballman uses Windows and may help you find the >>> differences. >> >> It was also failing on MacOS. And it failed a different way on another >> Windows version. That's why I removed the test for the deterministic shuffle. >> >>> I tend to agree with your `I think it's just a case where Windows' >>> algorithm for std::mt19937 is subtly different than the one for Linux.` >> >> The only other option would be to add the `EXPECT_*` stuff on one platform >> (like Linux). I suppose that would be better than nothing. > > Here's what has me confused... You're using `std::mt19937` which is a very > specific random number algorithm and you're giving it the same seed. I don't > think we *should* be getting different behavior from the random number > generator across platforms. The issue is the call to `std::shuffle` (see > https://en.cppreference.com/w/cpp/algorithm/random_shuffle#Notes), so I think > using `llvm::shuffle` will fix all the tests on all the platforms. Okay. So I'll make this change and submit it during a time when there isn't a lot of activity so that I can revert it if it fails without annoying too many people. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121556/new/ https://reviews.llvm.org/D121556 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
