steven_wu added a comment. In D141625#4100790 <https://reviews.llvm.org/D141625#4100790>, @dblaikie wrote:
> 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. No worry. Thanks for the thorough review and feedback. Maybe clang will have a tool to visualize clang binary module some day. I will go ahead and commit this. 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
