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

This revision was automatically updated to reflect the committed changes. Closed by commit rG0480748ea672: [DeclContext] Sort the Decls before adding into DeclContext (authored by steven_wu). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141625/new/

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

steven_wu added a comment. In D141625#4100790 , @dblaikie wrote: > In D141625#4100762 , @steven_wu > wrote: > >> I don't think it is feasible with current tool to write a test that check >> semantically the orde

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

dblaikie added a comment. In D141625#4100762 , @steven_wu wrote: > I don't think it is feasible with current tool to write a test that check > semantically the order of decls in clang module. (Let me know if that was > wrong). The current test already u

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

dblaikie added a comment. In D141625#4100660 , @akyrtzi wrote: >> would be great to test it more in a semantic way if possible > > Keep in mind that the specific order of the decls doesn't matter for the > purposes of this test, what matters is that the

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

steven_wu added a comment. I don't think it is feasible with current tool to write a test that check semantically the order of decls in clang module. (Let me know if that was wrong). The current test already unfortunately relies on the module layout assuming `op13` is the field for anonymous de

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

akyrtzi added a comment. > would be great to test it more in a semantic way if possible Keep in mind that the specific order of the decls doesn't matter for the purposes of this test, what matters is that the order is the same every time for the same input. I personally think that the addition

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

dblaikie added a comment. In D141625#4095197 , @steven_wu wrote: >> 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 m

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

steven_wu added a comment. > 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 i

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

dblaikie added a comment. In D141625#4094942 , @steven_wu wrote: > In D141625#4094837 , @dblaikie > wrote: > >> In D141625#4094742 , @steven_wu >> wrote: >> >>> In D1416

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

steven_wu added a comment. In D141625#4094837 , @dblaikie wrote: > In D141625#4094742 , @steven_wu > wrote: > >> In D141625#4067225 , @dblaikie >> wrote: >> >>> In D1416

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

dblaikie added a comment. In D141625#4094742 , @steven_wu wrote: > In D141625#4067225 , @dblaikie > wrote: > >> In D141625#4066961 , @steven_wu >> wrote: >> >>> No, reve

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

steven_wu added a comment. In D141625#4067225 , @dblaikie wrote: > In D141625#4066961 , @steven_wu > wrote: > >> No, reverse iteration will not break diff test for a small number of decls. >> Everything will be

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

dblaikie added a comment. In 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 y

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

steven_wu added a comment. 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. Current test will fail early in reverse iteration and will fail in the end statistically for forward iteration. Will pass in all configur

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

dblaikie added a comment. In D141625#4066681 , @steven_wu wrote: > In D141625#4066466 , @dblaikie > wrote: > >> In D141625#4066362 , @steven_wu >> wrote: >> >>> @dblaiki

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

steven_wu updated this revision to Diff 490616. steven_wu added a comment. Update tests according to feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141625/new/ https://reviews.llvm.org/D141625 Files: clang/lib/Parse/ParseDecl.cpp clang

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

steven_wu added a comment. In D141625#4066466 , @dblaikie wrote: > In D141625#4066362 , @steven_wu > wrote: > >> @dblaikie Do you have any objection for committing the patch as it is? I >> don't think reverse it

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

dblaikie added a comment. In D141625#4066362 , @steven_wu wrote: > @dblaikie Do you have any objection for committing the patch as it is? I > don't think reverse iteration test is a proper way to test for this specific > bug. I think reverse iteration

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

steven_wu added a comment. @dblaikie Do you have any objection for committing the patch as it is? I don't think reverse iteration test is a proper way to test for this specific bug. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141625/new/ https:/

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

steven_wu added a comment. In D141625#4060460 , @dblaikie wrote: > In D141625#4059486 , @steven_wu > wrote: > >> @dblaikie Do we have any bots running reverse iteration? > > Hmm, not that I can see/find at a quic

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

dblaikie added a subscriber: colinl. dblaikie added a comment. In D141625#4059486 , @steven_wu wrote: > @dblaikie Do we have any bots running reverse iteration? Hmm, not that I can see/find at a quick glance, which is unfortunate. @mgrang Are you still

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

steven_wu added a comment. In D141625#4059540 , @tschuett wrote: > EXPANSIVE_CHECKS will reshuffle the llvm::sort input: > https://lists.llvm.org/pipermail/llvm-dev/2018-April/122576.html This is a slightly different concern. The problem to catch is ite

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

tschuett added a comment. EXPANSIVE_CHECKS will reshuffle the llvm::sort input: https://lists.llvm.org/pipermail/llvm-dev/2018-April/122576.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141625/new/ https://reviews.llvm.org/D141625 __

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

steven_wu added a comment. @dblaikie Do we have any bots running reverse iteration? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141625/new/ https://reviews.llvm.org/D141625 ___ cfe-commits mailing list

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

steven_wu added a comment. In 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 `DeclC

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

dblaikie added a comment. In 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'

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

steven_wu added a comment. Actually, sorting in `numberAnonymousDeclsWithin` doesn't work for some reasons. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141625/new/ https://reviews.llvm.org/D141625 ___

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

steven_wu updated this revision to Diff 489587. steven_wu added a comment. Minor touch up on the test case. Also add some comments in test to explain what is being tested. I don't measure any performance regression from `-fsyntax-only` on Foundation.h but if there are some other performance ben

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

vsapsai accepted this revision. vsapsai added a comment. Confirm that for me on macOS without the fix the test is failing every time, so the test seems to be totally sufficient. Comment at: clang/lib/Parse/ParseDecl.cpp:6997 + D2->getLocation().getRawEncoding(); +

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

steven_wu updated this revision to Diff 489150. steven_wu added a comment. @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

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

steven_wu added a comment. In D141625#4052869 , @rsmith wrote: > In D141625#4052820 , @dblaikie > wrote: > >> Yeah, might be nice to document where the instability comes from - if it >> comes from a DenseMap or

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

akyrtzi added a comment. Yeah, you mainly need more than 32 decls to exceed the small storage of `Scope::DeclSetTy`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141625/new/ https://reviews.llvm.org/D141625 __

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

rsmith added a comment. In D141625#4052820 , @dblaikie wrote: > Yeah, might be nice to document where the instability comes from - if it > comes from a DenseMap or similar, then a test that fails either in forward or > reverse iteration mode would be ni

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

dblaikie added a comment. Yeah, might be nice to document where the instability comes from - if it comes from a DenseMap or similar, then a test that fails either in forward or reverse iteration mode would be nice to have. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

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

akyrtzi accepted this revision. akyrtzi added a comment. This revision is now accepted and ready to land. Is it impractical to add a test for this? Otherwise LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141625/new/ https://reviews.llvm.org/D

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

steven_wu created this revision. steven_wu added reviewers: jansvoboda11, akyrtzi, benlangmuir, vsapsai, rnk, dblaikie. Herald added subscribers: ributzka, mgrang. Herald added a project: All. steven_wu requested review of this revision. Herald added a project: clang. Fix a non-deterministic issu