dblaikie added a comment. In D141625#4100762 <https://reviews.llvm.org/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 unfortunately relies on the module layout > assuming `op13` is the field for anonymous declaration number. Sure enough - having these things have semantic identifiers rather than numbers would help. Ah, perhaps I'm just confused - I'm not sure why a similar test, that tested a different order of the op13 fields wouldn't've shown a failure without reverse iteration and then failed with reverse iteration (or the other way around) - and then could be updated with a different ordering with the fix. Sorry, I'm clearly not making much sense here. Yes, I think the test should be reduced further (while still showing the failure in either forward or reverse iteration - but, yes, losing coverage in the real world rehashing situation) it'd make the test shorter and more maintainable (to @akyrtzi's point about future changes that introduce benign reordering) but it's not the worst example of long tests to tickle hash instability. *shrug* I'm not insisting on it/blocking this patch from going forward without addressing this issue. Carry on. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141625/new/ https://reviews.llvm.org/D141625 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
