[PATCH] D121556: [randstruct] Add randomize structure layout support

Fangrui Song via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 11 16:59:41 PDT 2022


MaskRay added a comment.

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.

I wondered why the test did not fail again when you re-landed it. Now I see: you simply removed all order checks like `EXPECT_EQ(Expected, getFieldNamesFromRecord(RD));`
The tests seem overly relaxing and no longer serve the original purposes to catch errors (if the algorithm is changed to not randomize at all, I suspect the tests will pass as well).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121556/new/

https://reviews.llvm.org/D121556



More information about the cfe-commits mailing list