[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

Tue Jan 31 15:38:01 PST 2023

dblaikie added a comment.

In D141625#4094942 <https://reviews.llvm.org/D141625#4094942>, @steven_wu wrote:

> In D141625#4094837 <https://reviews.llvm.org/D141625#4094837>, @dblaikie wrote:
>
>> In D141625#4094742 <https://reviews.llvm.org/D141625#4094742>, @steven_wu wrote:
>>
>>> In D141625#4067225 <https://reviews.llvm.org/D141625#4067225>, @dblaikie wrote:
>>>
>>>> In D141625#4066961 <https://reviews.llvm.org/D141625#4066961>, @steven_wu wrote:
>>>>
>>>>> No, reverse iteration will not break diff test for a small number of decls. Everything will be in reverse order so it is the same.
>>>>
>>>> Hmm, I'm not sure I'm following why that is - could you explain this in more detail? The usual problem is that, say, we're outputting based on an unstable order - even two items would be enough to cause a test of that to fail in either forward or reverse iteration mode until the order is stabilized.
>>>>
>>>> Is that not the case here? Is there some interaction between iteration order that's part of the nondeterminism here?
>>>
>>> In order to make a test that will break before the change with reverse iteration, the test needs to check that the decls/records are serialized into the module in a pre deterministic order. It can't be just diff the output of two runs with a small input because small input will not overflow the smallptrset, thus the reverse iteration outputs from two runs will very likely to be identical, just in a different order from forward iteration.
>>
>> Sure, I think I'm with you there - but the current test checks that the decls/records are serialized into the module in a pre-deterministic order, right? So it doesn't seem like a reverse iteration-failing test would be more involved/brittle/less robust than the test being added here?
>
> Yes, exactly, I added file check so that this test is going to fail for reverse iteration. What I also want is keep the size of the test case since it not really enormous so this test also has a good chance of failing without reverse iteration.

Sorry, I'm still not really following - OK, sounds like you're saying this test does fail at HEAD/without this patch in reverse iteration mode, and is a bit larger than would be minimally necessary to achieve that, but also might fail at HEAD without reverse iteration, providing somewhat more testing than if it were fully minimized/only caught in reverse.

Fair enough -I don't think it's the right tradeoff, but I'm glad it's stable/provides that coverage.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625