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

Thu Feb 2 15:05:23 PST 2023

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