Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix panic when eliding invalid constant cases #46

Merged
merged 1 commit into from
Feb 23, 2017

Conversation

benpaxton-hf
Copy link
Contributor

+, >, <, <=, and >= require typechecking of both operands
simultaneously; constant-operand eliding did not perform this check,
causing a panic.

In the below examples, conditional.go is a test program I wrote when investigating this library; it is essentially a minimal REPL for govaluate.

Before:

$ go run conditional.go 
> 1 > true
panic: interface conversion: interface is bool, not float64

goroutine 1 [running]:
panic(0x4e0180, 0xc42001a0c0)
	/usr/local/go/src/runtime/panic.go:500 +0x1a1
github.com/Knetic/govaluate.gtStage(0x4d5ac0, 0xc42000a188, 0x4d5900, 0xc42000a198, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/repos/go/src/github.com/Knetic/govaluate/evaluationStage.go:103 +0xb7
github.com/Knetic/govaluate.elideStage(0xc4200a00f0, 0xc4200a00a0)
	/repos/go/src/github.com/Knetic/govaluate/stagePlanner.go:662 +0x254
[...]
exit status 2

After:

$ go run conditional.go 
> 1 > true
Evaluate: Value '1' cannot be used with the comparator '>', it is not a number
> 

The error message is still incorrect, but it's better than a panic. Likely the error message should be something along the lines of "Values '1' and 'true' cannot be used with the comparator '>'".

+, >, <, <=, and >= require typechecking of both operands
simultaneously; constant-operand eliding did not perform this check,
causing a panic.
@Knetic Knetic self-assigned this Feb 23, 2017
@Knetic
Copy link
Owner

Knetic commented Feb 23, 2017

Looks great to me!

Thanks for using the library, and taking the time to put together a PR!

@Knetic Knetic merged commit 50a86a4 into Knetic:master Feb 23, 2017
@benpaxton-hf benpaxton-hf deleted the constant-eliding branch February 24, 2017 14:22
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