[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext
Steven Wu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
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
More information about the cfe-commits
mailing list