rpn(1) #36

Closed
emma wants to merge 0 commits from rpn into main
Owner

Closes #21

Closes #21
emma added the
enhancement
label 2024-02-01 00:42:49 -07:00
emma self-assigned this 2024-02-01 00:42:49 -07:00
emma added 3 commits 2024-02-01 00:42:49 -07:00
emma added 1 commit 2024-02-01 00:58:22 -07:00
emma requested review from trinity 2024-02-01 00:58:27 -07:00
emma added 1 commit 2024-02-01 01:18:48 -07:00
emma added 1 commit 2024-02-01 01:27:17 -07:00
emma added 1 commit 2024-02-01 01:34:15 -07:00
trinity reviewed 2024-02-01 07:11:48 -07:00
trinity left a comment
Owner

I'd like to see the bitwise operators and floor division from #21 included but all in all i think this is a good implementation and better than I could have done (or even intended to do).

I'd like to see the bitwise operators and floor division from #21 included but all in all i think this is a good implementation and better than I could have done (or even intended to do).
Makefile Outdated
@ -58,3 +58,3 @@
libgetopt: src/getopt-rs/lib.rs
$(RUSTC) $(RUSTCFLAGS) --crate-type=lib --crate-name=getopt \
$(RUSTC) $(RUSTFLAGS) --crate-type=lib --crate-name=getopt \
Owner

Can this change be made independent of rpn(1) or is it necessary for rpn(1) in particular?

Can this change be made independent of rpn(1) or is it necessary for rpn(1) in particular?
Author
Owner

No reason to walk back the commit now that it’s been made

No reason to walk back the commit now that it’s been made
emma marked this conversation as resolved
src/rpn.rs Outdated
@ -0,0 +69,4 @@
}
#[derive(Debug, Clone)]
struct EvaluationError {
Owner

Can we emit argv[0] before printing errors?

Can we emit argv[0] before printing errors?
Author
Owner

If the program does not do that on your machine, you are encountering a bug that I cannot reproduce.

If the program does not do that on your machine, you are encountering a bug that I cannot reproduce.
Owner

I haven't run it on my machine, I just didn't see that it did that.

I haven't run it on my machine, I just didn't see that it did that.
trinity marked this conversation as resolved
src/rpn.rs Outdated
@ -0,0 +85,4 @@
// str_to_calc_type converts a string to an optional `CalcType`.
fn str_to_calc_type(string: &str) -> Option<CalcType> {
let as_int = string.parse::<f64>();
Owner

Will this work for non-decimal integers, e.g. 0x10 (16), 0o10 (8), 0b1 (2)?

Will this work for non-decimal integers, e.g. 0x10 (16), 0o10 (8), 0b1 (2)?
Author
Owner

Implementing this would be at the cost of storing numbers as signed 32-bit integers before converting them to 64-bit floats, which would lessen the maximum size of the integers we can take.

Implementing this would be at the cost of storing numbers as signed 32-bit integers before converting them to 64-bit floats, which would lessen the maximum size of the integers we can take.
emma marked this conversation as resolved
src/rpn.rs Outdated
@ -0,0 +106,4 @@
}
}
fn eval(
Owner

I would prefer evaluate() only because Python, shell, and Lisp have beaten terror at the sight of the abbreviation into me.

I would prefer evaluate() only because Python, shell, and Lisp have beaten terror at the sight of the abbreviation into me.
emma marked this conversation as resolved
src/rpn.rs Outdated
@ -0,0 +132,4 @@
};
match x {
Add | Divide | Multiply | Power | Subtract | Modulo => {
Owner

Is this just every CalcType besides Val? Is there a better way to write that, e.g. match except Val?

(this isn't a rhetorical question i don't know rust)

Is this just every CalcType besides Val? Is there a better way to write that, e.g. match *except* Val? (this isn't a rhetorical question i don't know rust)
Author
Owner
		match x {
			Val(x_) => stack.push_back(x_),
			_ => {
				ops.push_back(x)
			},
		}
	}
``` match x { Val(x_) => stack.push_back(x_), _ => { ops.push_back(x) }, } } ```
emma marked this conversation as resolved
src/rpn.rs Outdated
@ -0,0 +156,4 @@
Multiply => &stack.push_back(y * x),
Divide => &stack.push_back(y / x),
Power => &stack.push_back(x.powf(*y)),
Modulo => &stack.push_back(y % x),
Owner

I chipped in my thought on making sure rpn(1) functions like HP and Elektronika calculators when operating and this implementation satisfies me.

I chipped in my thought on making sure rpn(1) functions like HP and Elektronika calculators when operating and this implementation satisfies me.
emma marked this conversation as resolved
src/rpn.rs Outdated
@ -0,0 +183,4 @@
let argv = args().collect::<Vec<String>>();
let mut stack = VecDeque::new();
let mut buf = String::new();
let precision = (-f64::EPSILON.log10() * PRECISION_MOD).ceil() as usize;
Owner

What's happening here?

What's happening here?
Author
Owner

Setting the float precision for rounding errors based on the machine epsilon

Setting the float precision for rounding errors based on the [machine epsilon](https://en.wikipedia.org/wiki/Machine_epsilon)
emma marked this conversation as resolved
emma added 2 commits 2024-02-01 10:47:00 -07:00
emma added 1 commit 2024-02-01 11:03:04 -07:00
Author
Owner

I’m currently writing rpn(1) and will be writing rpn(7) for a comprehensive guide to reverse polish notation as soon as I get around to it.

I’m currently writing `rpn(1)` and will be writing `rpn(7)` for a comprehensive guide to reverse polish notation as soon as I get around to it.
emma added 1 commit 2024-02-01 11:37:10 -07:00
emma added 1 commit 2024-02-01 12:38:46 -07:00
emma added 1 commit 2024-02-01 13:07:24 -07:00
emma added 1 commit 2024-02-01 23:33:26 -07:00
emma added 1 commit 2024-02-01 23:34:53 -07:00
emma added 1 commit 2024-02-01 23:36:19 -07:00
trinity reviewed 2024-02-02 07:15:44 -07:00
trinity left a comment
Owner

good stuff

good stuff
@ -0,0 +7,4 @@
.SH NAME
npc \(en reverse polish notation evaluation
Owner

npc?

npc?
emma marked this conversation as resolved
@ -0,0 +11,4 @@
.SH SYNOPSIS
rpn [integers...] [operations...]
Owner

Integers and operations can be mixed. I would classify an integer as an operation, too - the push operation.

Integers and operations can be mixed. I would classify an integer as an operation, too - the push operation.
emma marked this conversation as resolved
docs/rpn.1 Outdated
@ -0,0 +13,4 @@
rpn [integers...] [operations...]
.SH DESCRIPTION
Owner

"Rpn evaluates a given reverse polish notation expression according to the conventions defined in rpn(7), printing the last item on its stack on completion."

"Rpn evaluates a given reverse polish notation expression according to the conventions defined in rpn(7), printing the last item on its stack on completion."
emma marked this conversation as resolved
docs/rpn.1 Outdated
@ -0,0 +22,4 @@
.SH STANDARD INPUT
If rpn is passed arguments, it interprets those arguments as an expression to
be evaluated. Otherwise, it reads characters from standard input to add to the
Owner

"Otherwise, it reads a newline-delimited series of numbers and operations from standard input."

"Otherwise, it reads a newline-delimited series of numbers and operations from standard input."
Author
Owner

They aren’t necessarily newline-delimited.

They aren’t necessarily newline-delimited.
Owner

whitespace-delimited

whitespace-delimited
emma marked this conversation as resolved
docs/rpn.1 Outdated
@ -0,0 +33,4 @@
.SH CAVEATS
Due to the complexity of integer storage in memory, rpn is only capable of
parsing decimal integers.
Owner

I would classify this as a bug, not a caveat, and refrain from mentioning implementation details in the man page.

I would classify this as a bug, not a caveat, and refrain from mentioning implementation details in the man page.
Owner

It looks like dc(1) historically also had this issue. I'd really like to not have it though and I might try to fix this myself.

It looks like dc(1) historically also had this issue. I'd really like to not have it though and I might try to fix this myself.
emma marked this conversation as resolved
docs/rpn.1 Outdated
@ -0,0 +37,4 @@
Additionally, due to the laws of physics, floating-point math can only be as
precise as slightly less than the machine epsilon of the hardware on which rpn
is running.
Owner

I don't think this caveat is needed here, maybe there should be a float(7) that explains IEEE-754 and its caveats.

I don't think this caveat is needed here, maybe there should be a float(7) that explains IEEE-754 and its caveats.
emma marked this conversation as resolved
docs/rpn.1 Outdated
@ -0,0 +43,4 @@
POSIX has its own calculator in the form of bc(1p), which uses standard input
for its calculations. Pair the clunkiness of piping expressions into it and its
use of standard notation, it was clear what rpn should be.
Owner

This reads weirdly. I would say:

"An infix notation calculation utility, bc(1p), is specified in POSIX, but doesn't accept expressions in arguments, requiring printf(1p) to be piped into bc(1p) in order to be used non-interactively. A dc(1) pre-dates the standardized bc(1p), the latter originally being a preprocessor for the former, and was included in UNIX v2 onward. While it implements reverse polish notation it still suffers from being unable to accept an expression as an argument."

See Early UNIX history (which says dc(1) was implemented by 1970) and the UNIX v2 source tree.

This reads weirdly. I would say: "An infix notation calculation utility, bc(1p), is specified in POSIX, but doesn't accept expressions in arguments, requiring printf(1p) to be piped into bc(1p) in order to be used non-interactively. A dc(1) pre-dates the standardized bc(1p), the latter originally being a preprocessor for the former, and was included in UNIX v2 onward. While it implements reverse polish notation it still suffers from being unable to accept an expression as an argument." See [Early UNIX history](https://www.bell-labs.com/usr/dmr/www/hist.html) (which says dc(1) was implemented by 1970) and [the UNIX v2 source tree](https://www.tuhs.org/cgi-bin/utree.pl?file=V2/cmd).
emma marked this conversation as resolved
docs/rpn.1 Outdated
@ -0,0 +47,4 @@
There are no mathematics in the qi(1) shell because it was decided early on that
math was the job of a specific tool and not the shell itself. Thus, rpn was
born.
Owner

This probably belongs in a qi(1) man page, not here.

This probably belongs in a qi(1) man page, not here.
emma marked this conversation as resolved
@ -0,0 +68,4 @@
Modulo,
Val(f64),
Empty,
}
Owner

If this were C I would put the sentry value first so validity is just a measure of whether or not the enum is 0. I don't know if Rust works that way but it's a thought. It may also make more sense conceptually if Empty and Val are the zeroth and first enumerations respectively.

If this were C I would put the sentry value first so validity is just a measure of whether or not the enum is 0. I don't know if Rust works that way but it's a thought. It may also make more sense conceptually if Empty and Val are the zeroth and first enumerations respectively.
Author
Owner

What does it mean for an enum to be zero?

What does it mean for an enum to be zero?
Owner

In C - I can't speak as to how Rust does it - enums are basically #defines as they replace a given symbol with another symbol, though unlike #defines the symbol remains in the code rather than being a text replacement (which at some point probably mattered for debugging but I don't notice the difference). The enum being zero means a given value (e.g. Empty) evaluates to 0.

In C - I can't speak as to how Rust does it - enums are basically `#define`s as they replace a given symbol with another symbol, though unlike `#define`s the symbol remains in the code rather than being a text replacement (which at some point probably mattered for debugging but I don't notice the difference). The enum being zero means a given value (e.g. `Empty`) evaluates to `0`.
Author
Owner

I don’t think that’s how it works in Rust—also, if I recall correctly, as far as I’ve seen in other people’s code, the error variant is usually last, so I feel there is some convention here. I will look for some examples of this to confirm.

I don’t think that’s how it works in Rust—also, if I recall correctly, as far as I’ve seen in other people’s code, the error variant is usually last, so I feel there is some convention here. I will look for some examples of this to confirm.
emma marked this conversation as resolved
emma added 2 commits 2024-02-02 14:17:24 -07:00
Author
Owner

With further testing, I’ve realized the eval() function needs to be rewritten practically from scratch to prevent edge cases in the parsing.

With further testing, I’ve realized the `eval()` function needs to be rewritten practically from scratch to prevent edge cases in the parsing.
emma added 1 commit 2024-02-02 15:37:31 -07:00
trinity reviewed 2024-02-02 16:38:03 -07:00
src/rpn.rs Outdated
@ -0,0 +77,4 @@
"-" | "" => Subtract,
"*" | "×" => Multiply,
"/" | "÷" => Divide,
"^" => Power,
Owner

I would use ** for exponentiation as ^ is conventionally used for bitwise xor.

I would use `**` for exponentiation as `^` is conventionally used for bitwise xor.
Owner

I think ** should be allowed, it's a common name for the operation. It doesn't have to be the only name but it should be an option.

I think `**` should be allowed, it's a common name for the operation. It doesn't have to be the only name but it should be an option.
emma marked this conversation as resolved
emma added 1 commit 2024-02-02 19:14:52 -07:00
emma added 1 commit 2024-02-02 19:17:31 -07:00
emma added 1 commit 2024-02-02 20:12:49 -07:00
trinity reviewed 2024-02-02 21:00:08 -07:00
src/rpn.rs Outdated
@ -0,0 +148,4 @@
Val(v) => stack.push_back(v),
Invalid => {
return Err(EvaluationError {
message: format!("{}: Invalid token", n)
Owner

n can only ever be Invalid.

`n` can only ever be `Invalid`.
emma marked this conversation as resolved
emma added 1 commit 2024-02-02 21:06:08 -07:00
emma added 1 commit 2024-02-02 23:25:26 -07:00
Author
Owner

Next goal: I would like to include error codes in the EvaluationError struct.

Next goal: I would like to include error codes in the EvaluationError struct.
emma added 1 commit 2024-02-03 17:21:25 -07:00
emma changed title from WIP: rpn(1) to rpn(1) 2024-02-06 23:24:48 -07:00
emma added 1 commit 2024-02-06 23:30:24 -07:00
emma removed review request for trinity 2024-02-06 23:31:33 -07:00
emma requested review from trinity 2024-02-06 23:31:36 -07:00
trinity approved these changes 2024-02-07 07:05:32 -07:00
trinity left a comment
Owner

I've suggested minor fixes but the code looks alright.

I've suggested minor fixes but the code looks alright.
docs/rpn.1 Outdated
@ -0,0 +18,4 @@
.SH DESCRIPTION
Rpn parses and and evaluates reverse polish notation expressions either from the
standard input or by parsing its arguments. See the STANDARD INPUT section.
Owner

"either from given arguments, or, in their absence, standard input" - right now you have "parse" twice in there which feels weird.

"either from given arguments, or, in their absence, standard input" - right now you have "parse" twice in there which feels weird.
emma marked this conversation as resolved
docs/rpn.1 Outdated
@ -0,0 +22,4 @@
Upon evaluation, rpn will print the resulting number on the stack to the
standard output. Any further specified numbers will be placed on the stack
following the last outputted number.
Owner

I believe the past tense of "output" is "output" - also, I'm having a hard time understanding what you're trying to convey here - shouldn't rpn print the last number on the stack? It sounds like it prints the last result of any algebraic operations but ignores further given numbers.

I believe the past tense of "output" is "output" - also, I'm having a hard time understanding what you're trying to convey here - shouldn't rpn print the last number on the stack? It sounds like it prints the last result of any algebraic operations but ignores further given numbers.
Author
Owner

According to Merriam-Webster [1], the verb outputted is the past tense form of output. I want to change this paragraph regardless because it is a bit clunky.

[1] https://www.merriam-webster.com/dictionary/outputted

According to Merriam-Webster [1], the verb outputted is the past tense form of output. I want to change this paragraph regardless because it is a bit clunky. [1] https://www.merriam-webster.com/dictionary/outputted
emma marked this conversation as resolved
docs/rpn.1 Outdated
@ -0,0 +28,4 @@
.SH STANDARD INPUT
If rpn is passed arguments, it interprets them as an expression to be evaluated.
Owner

"If arguments are passed to rpn" reads cleaner to me but I can't put my finger on why.

"If arguments are passed to rpn" reads cleaner to me but I can't put my finger on why.
emma marked this conversation as resolved
docs/rpn.1 Outdated
@ -0,0 +43,4 @@
with the IEEE Standard for Floating Point Arithmetic (IEEE 754), floating-point
arithmetic has rounding errors. This is somewhat curbed by using the
second-highest float that can be represented in line with this standard to round
numbers to before outputting.
Owner

It's less in accord with IEEE 754 and more that we are applying IEEE 754 (or, more specifically, Rust is - and even then, it's the processor itself I think that is doing the floating point operations). I would say "Due to IEEE 754 (the IEEE Standard for Floating Point Arithmetic) limitations, rounding errors may occur. And not "in line" with the standard but simply "that can be represented".

What's the second-highest float? This man page leaves me unclear on how much I should lean on rpn for fractional values.

It's less *in accord* with IEEE 754 and more that we are applying IEEE 754 (or, more specifically, Rust is - and even then, it's the processor itself I think that is doing the floating point operations). I would say "Due to IEEE 754 (the IEEE Standard for Floating Point Arithmetic) limitations, rounding errors may occur. And not "in line" with the standard but simply "that can be represented". What's the second-highest float? This man page leaves me unclear on how much I should lean on rpn for fractional values.
Author
Owner

Due to [...] the way floats are represented in accordance with the IEEE Standard [...]

This does not imply any work on our part. The floats are represented in accordance with IEEE standards in Rust, which is implicit in the sentence. It is equivalent to saying:

Due to [...] the way floats are represented in Rust [...]

> Due to [...] the way floats are represented in accordance with the IEEE Standard [...] This does not imply any work on our part. The floats are represented in accordance with IEEE standards *in Rust*, which is implicit in the sentence. It is equivalent to saying: > Due to [...] the way floats are represented in Rust [...]
emma marked this conversation as resolved
@ -0,0 +48,4 @@
.SH RATIONALE
An infix notation calculation utility, bc(1p), is included in the POSIX
standard, but it doesnt accept expressions as arguments; in scripts, any
Owner

I don't like the use of the Unicode apostrophe here because it could mess with literal text lookups. Nobody likes a No occurrences of "doesn't" because of fancy rune voodoo.

I don't like the use of the Unicode apostrophe here because it could mess with literal text lookups. Nobody likes a `No occurrences of "doesn't"` because of fancy rune voodoo.
emma marked this conversation as resolved
src/rpn.rs Outdated
@ -0,0 +99,4 @@
Multiply => "multiplication",
Divide => "division",
Power => "exponentiation",
Floor => "floor divion",
Owner

division

division
emma marked this conversation as resolved
emma added 1 commit 2024-02-07 18:52:44 -07:00
emma added 1 commit 2024-02-07 19:12:27 -07:00
emma added 1 commit 2024-02-07 19:13:02 -07:00
emma added 1 commit 2024-02-07 19:18:32 -07:00
emma closed this pull request 2024-02-07 19:24:50 -07:00

Pull request closed

Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: bonsai/harakit#36
No description provided.