ck3-tiger icon indicating copy to clipboard operation
ck3-tiger copied to clipboard

Fix Loc ordering

Open BrknRobot opened this issue 5 months ago • 4 comments

Loc order was not stable between runs as link_index is non-deterministic. This implements stable ordering for Loc, giving reports consistent ordering between runs.

To avoid repeated MACRO_MAP traversals when comparing expanded log report pointers, Loc is split into Loc and LocStack with Loc excluding link_idx

BrknRobot avatar Aug 11 '25 19:08 BrknRobot

So... I don't like the introduction of LocStack. Token and Loc are fundamental to tiger and I don't want to complicate that.

As an alternative for the performance benefit, I think you could set link_idx to None when expanding the pointers. It makes sense because the chains are being broken there.

amtep avatar Aug 12 '25 09:08 amtep

Are you more concerned with the code complexity or interface complexity? I see value in allowing the code to be explicit about whether it's expecting to handle a location in a file, or a whole stack trace. But it's up to you. Other places that would make location comparisons like DbItem insertion should be working with Loc's that have link_idx set to None, or at the very least, Loc's that differ at the top of the stack rather than some number of links down I think, so it's more about explicit expectations than perf for the rest of the codebase. I just didn't go through and adjust any other function signatures or structs accordingly because I didn't want to introduce any potential behaviour changes outside of the specific area I was testing.

BrknRobot avatar Aug 12 '25 16:08 BrknRobot

Which one is conceptual complexity? :) I want Token and Loc and Block to remain the fundamental concepts used by tiger. I think having two kinds of Loc is going to upset that foundation for very little benefit.

Other places that would make location comparisons like DbItem insertion should be working with Loc's that have link_idx set to None, or at the very least, Loc's that differ at the top of the stack rather than some number of links down I think, so it's more about explicit expectations than perf for the rest of the codebase.

Item insertion works with Tokens, so applying this there would mean also splitting TokenWithLocStack from Token... it won't be pretty, and it's an example of how this extra conceptual complexity would work its way through the code base.

amtep avatar Aug 12 '25 20:08 amtep

Ahh, okay. I'm convinced. I'll put the Loc's back together

BrknRobot avatar Aug 12 '25 22:08 BrknRobot