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

Tue Jan 17 09:01:22 PST 2023

steven_wu added a comment.

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

> Actually, sorting in `numberAnonymousDeclsWithin` doesn't work for some reasons.

The reason for this doesn't work is `ASTWriter::WriteDeclContextLexicalBlock` also iterates on `DeclContext::decls()`, so there are at least two sorts needed in ASTWriter. I prefer the current implementation if there isn't any performance overhead since it makes the iterator on DeclContext to have stable order.

In D141625#4059186 <https://reviews.llvm.org/D141625#4059186>, @dblaikie wrote:

> In D141625#4053067 <https://reviews.llvm.org/D141625#4053067>, @steven_wu wrote:
>
>> @akyrtzi has the good idea. It is really hard to control `Decl*` to get values
>> to get an unstable iteration order from the small tests, going beyond 32 decls
>> to get out of SmallPtrSet's small model is much consistent.
>>
>> Add test.
>
> Could you make a smaller test (probably just a couple of decls) that fails with LLVM_ENABLE_REVERSE_ITERATION? Such a failure would be more reliable/simpler to reproduce, probably?

Sure but it is going to put a different requirement on the tests. Now ASTWriter only cares about a stable order for deterministic output so it is doing a diff on pcm. If changing to the reverse iterator test, this need to change to do FileCheck on a predetermined order.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625