softdevteam / grmtools

Rust grammar tool libraries and binaries
Other
507 stars 31 forks source link

Remove StIdxStorageT and replace it with StorageT #326

Closed ltratt closed 2 years ago

ltratt commented 2 years ago

The main bulk of this PR is in https://github.com/softdevteam/grmtools/commit/af8c85ede6bfafe4b6828063a3672519e1833b4c and partially ameliorates a long-time grmtools wart, which is that the maximum number of parsing states was hardcoded to a u16. This commit moves that to StoargeT maximum states. In one sense that overloads StorageT but at least the user now has some control over how many states grmtools can deal with.

Thoughts welcome: this is a part of the code I haven't looked at in some time!

ratmice commented 2 years ago

Well, this isn't really an area I'm very familiar with, and I've never shied away from adding generic type parameters (for better or worse). But it doesn't seem like adding another type parameter in this case would actually buy you much, partially because the trait bounds don't actually enforce these sizing invariants, so adding another would seem to come with additional dynamic checks.

One thing I notice that we could use to reduce the StorageT bounds, is something like: https://www.worthe-it.co.za/blog/2017-01-15-aliasing-traits-in-rust.html

But I kind of don't think that adding these aliasing traits, would actually tip this towards adding generic type parameters. Perhaps we could consider that in a separate patch though?

ltratt commented 2 years ago

I did not know about the trait alias trick! I agree that it's probably something to consider in a separate tidy-up PR.

ratmice commented 2 years ago

Cool, well please squash, if you feel it is ready.

ltratt commented 2 years ago

I think this is an improvement (albeit there's more to be done!). Squashed.

ratmice commented 2 years ago

bors r+

bors[bot] commented 2 years ago

Build succeeded: