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

Thu Feb 2 12:54:17 PST 2023

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