Skip to content

Conversation

@BrknRobot
Copy link
Contributor

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

@amtep
Copy link
Owner

amtep commented Aug 12, 2025

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.

@BrknRobot
Copy link
Contributor Author

BrknRobot commented Aug 12, 2025

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.

@amtep
Copy link
Owner

amtep commented Aug 12, 2025

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.

@BrknRobot
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants