Initial code review

[charlesroddie]

Can we move to a PR system as it exposes good commenting functionality?

In lieu of this some initial comments:

• inline shouldn't be used everywhere. It should be used for parametrized types where the type system has problems or where specialization for performance is needed. Definitely doesn't make sense for non-generic functions.
• (|Alphabet|NonAlphabet|) -> can simplify to let isAlphabetic c =. "alphabet" should include alpha and beta :).
• [<CompiledName "Delimiters">]let delimiters -> let Delimiters
• Can flatten type Options = { Delimiters: AliasMap<string, Delimiter> }, removing record.
• AliasMap is not good. Pretending to be immutable doesn't work as an idea. Performance will be terrible. The API is too extensive and should be reduced to a minimum. This can be a lot simpler and does look like an F# experiment.

Re: value vs refernce types, I expect there will be some threshold below which value types will perform better (e.g. ValueOption<char*char>) and above which passing by reference will be more efficient. Not sure where that theshold is exactly though.

[Happypig375]

• Please explain "PR system" a bit.
• Agreed on no-inline for larger helper functions. However, I still think inline would be appropriate for one-liners as they provide eliminations of allocations of e.g. FSharpFuncs.
• Pattern matching with Alphabet and NonAlphabets just look nicer than when clauses. ((NonAlphabet & c)::cs) They are primarily used for LaTeX command parsing, so Greek wouldn't apply here.
• I'm still thinking of a reason to deviate from the F# naming convention. Thoughts?
• Options will include more maps like CustomLaTeXCommands and Colors, so it shouldn't be flattened now.
• I'll look into benchmarking AliasMap, as there are no real data to discuss as of now. Also, I don't really like when collection types don't implement standard interfaces. (Looking at you, MatchCollection.) It's juat too surprising to the user.
• I remember I read that a Microsoft recommendation is to limit value types to 8 bytes or less. I think all struct types in MathDisplay don't exceed that. Correct me if I'm wrong.

[charlesroddie]

Please explain "PR system" a bit.

Perhaps a dev branch, with a PR from this into master allowing comments, merged from time to time?

Agreed on no-inline for larger helper functions. However, I still think inline would be appropriate for one-liners as they provide eliminations of allocations of e.g. FSharpFuncs.

Interesting performance tweak which we could consider and test if we are running into these hundreds of millions of times a second.

Pattern matching with Alphabet and NonAlphabets just look nicer than when clauses. ((NonAlphabet & c)::cs)

It's a more complex object: greater cognitive load, higher threshold for library contributions if we have these where more concrete approaches work equally well. I think we should allow active patterns selectively when they reduce complexity by expressing a concept that would be more complex to express concretely.

They are primarily used for LaTeX command parsing, so Greek wouldn't apply here.

OK

I'm still thinking of a reason to deviate from the F# naming convention. Thoughts?

Your annotation indicates you want to deviate from it. Just do it. Lower case convention is not important and being C#-friendly is a good enough reason to deviate from it. Having different compiled and used names creates unnecessary complexity.

Options will include more maps like CustomLaTeXCommands and Colors, so it shouldn't be flattened now.

OK

I remember I read that a Microsoft recommendation is to limit value types to 8 bytes or less. I think all struct types in MathDisplay don't exceed that. Correct me if I'm wrong.

This seems reasonable. That comment of mine wasn't advice to change anything, only an observation as you are using the new F# struct features and it's not that well understood when they help and when they don't.

[Happypig375]

The benchmark results are out! https://github.com/charlesroddie/MathAtom/blob/e93d3dc9337541326d59f47bb024829a1bf6713d/MathDisplay.Tests.Benchmarks/AliasMapBenchmark.fs#L43-L52 I think some of the difference might be from Map vs Dictionary differences, so I tried it out: https://github.com/charlesroddie/MathAtom/blob/91b846fee49a9ca2d58d27cb36d6acbe81e0dda1/MathDisplay.Tests.Benchmarks/MapBenchmark.fs#L57-L74 Maps perform worse than Dictionaries. Does this mean that Maps should be always avoided?

[charlesroddie]

Does this mean that Maps should be always avoided?

In this case you have a mutable API so backing it with a mutable structure makes sense.

When you want an immutable structure you can consider Map (balanced search tree) and dict (hash table, I think an immutable wrapper around Dictionary). BST vs hash table

[Happypig375]

So Dictionary is for inserting, deleting and searching, while Map is for sorting. Got it.