search

J-F-Liu / pom

PEG parser combinators using operator overloading without macros.
MIT License
496 stars 30 forks source link

Remove more unnecessary traits and generalize PartialEq usage #40

Closed afranchuk closed 3 years ago

afranchuk commented 4 years ago

These changes remove some unnecessary trait restrictions on the functions, and also generalize all uses of PartialEq (including the set implementation) to arbitrary types implementing PartialEq<T>. I think in most cases this introduces no breaking changes (and all tests still run and pass as before), however a larger corpus or test base may better confirm this. The only thing that makes me less sure is things like the change to sym which cause the returned parser to be a different type than types that are inferred from input arguments (i.e., the output type now is inferred from usage rather than from arguments, so it may be the case that existing code will need annotations to disambiguate). So to be safe it may be appropriate to put this toward a breaking API release.

J-F-Liu commented 4 years ago

I think the change of type parameter is counterintuitive, what's the benefit?

afranchuk commented 4 years ago

It is to allow someone to use sym/one_of/etc. with types that can be used for equality comparison but aren't necessarily the same type as the input stream.

For example, I needed it because my input stream is not bytes, but rather tokens with source information (looking like Source<Token>), and I wanted to be able to simply use (for example) sym(Token::OpenParen), where PartialEq<Source<Token>> is implemented for Token. Otherwise I'd have to create a Source<Token> with a nonsense source location and change PartialEq for Source<T> to never compare the actual source positions, making that less useful for other situations.

J-F-Liu commented 4 years ago

This use case may be better handled in some other way without complicating existing code.

afranchuk commented 4 years ago

That's all right by me; maybe simply as new functions?

J-F-Liu commented 3 years ago

@afranchuk I did a test and found that performance is not improved by using ErrorDelayed.

afranchuk commented 3 years ago

Whoops, I didn't intend for that to be part of this PR. However, it absolutely is improved. I profiled my application because it was getting slow; delaying formatting until it actually is needed (because many of those errors are dropped by upper alternative/one_of calls) makes a huge difference. In wall time my parsing went from ~5 seconds to 0.15 seconds.

afranchuk commented 3 years ago

However I'm going to move those commits into a separate branch.