Skip to content

Conversation

@glebm
Copy link

@glebm glebm commented Jun 10, 2025

Extracted from #374.

@NSoiffer
Copy link
Owner

I'm not seeing the why adding CanonicalizeContextPatternsOptions is an improvement. It actually adds a bit more code to the implementation. I only see one place where there is a minor shortening. The use of a structure maybe adds an extra indirection when running the code. Can you explain why you think this is important? Does it somehow help with your goal of having a stateless MathCAT?

@glebm
Copy link
Author

glebm commented Sep 12, 2025

It's not about the amount of code but about eliminating the duplication.

In the stateless PR, there is a new CanonicalizeContext.new_uncached function, which would otherwise also need to manually extract stuff from pref_manager:

impl CanonicalizeContext {
	pub fn new_uncached(pref_manager: &PreferenceManager) -> CanonicalizeContext {
		return CanonicalizeContext {
			patterns: Rc::new( CanonicalizeContextPatterns::new(
				&CanonicalizeContextPatternsOptions::from_prefs(pref_manager)) ),
		};
	}

The options struct also act as the cache key, making the equality comparison a simple ==, which is nice.

The use of a structure maybe adds an extra indirection when running the code

I don't think it does -- it's stored by value in CanonicalizeContextPatternsCache so the number of pointer indirection is the same as before.

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