Skip to content

Commit

Permalink
Fix: outdated, incorrect documentation about the order of precedence for
Browse files Browse the repository at this point in the history
  operator modulus `%`. See #3189
  • Loading branch information
josdejong committed Apr 29, 2024
1 parent da0c70e commit 599f4ee
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 6 deletions.
2 changes: 2 additions & 0 deletions HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
# unpublished changes since 12.4.2

- Fix: serialization of Units without a value, see #1240.
- Fix: outdated, incorrect documentation about the order of precedence for
operator modulus `%`. See #3189.


# 2024-04-24, 12.4.2
Expand Down
3 changes: 2 additions & 1 deletion docs/expressions/syntax.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,9 @@ Operators | Description
`!` | Factorial
`^`, `.^` | Exponentiation
`+`, `-`, `~`, `not` | Unary plus, unary minus, bitwise not, logical not
`%`, `mod` | percentage, modulus
See section below | Implicit multiplication
`*`, `/`, `.*`, `./`, `%`, `mod` | Multiply, divide, percentage, modulus
`*`, `/`, `.*`, `./` | Multiply, divide
`+`, `-` | Add, subtract
`:` | Range
`to`, `in` | Unit conversion
Expand Down
11 changes: 6 additions & 5 deletions src/expression/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,7 @@ export const createParse = /* #__PURE__ */ factory(name, dependencies, ({
}

/**
* multiply, divide, modulus
* multiply, divide
* @return {Node} node
* @private
*/
Expand Down Expand Up @@ -1102,7 +1102,7 @@ export const createParse = /* #__PURE__ */ factory(name, dependencies, ({
* @private
*/
function parseRule2 (state) {
let node = parsePercentage(state)
let node = parseModulusPercentage(state)
let last = node
const tokenStates = []

Expand All @@ -1125,7 +1125,7 @@ export const createParse = /* #__PURE__ */ factory(name, dependencies, ({
// Rewind once and build the "number / number" node; the symbol will be consumed later
Object.assign(state, tokenStates.pop())
tokenStates.pop()
last = parsePercentage(state)
last = parseModulusPercentage(state)
node = new OperatorNode('/', 'divide', [node, last])
} else {
// Not a match, so rewind
Expand All @@ -1147,11 +1147,11 @@ export const createParse = /* #__PURE__ */ factory(name, dependencies, ({
}

/**
* percentage or mod
* modulus and percentage
* @return {Node} node
* @private
*/
function parsePercentage (state) {
function parseModulusPercentage (state) {
let node, name, fn, params

node = parseUnary(state)
Expand All @@ -1160,6 +1160,7 @@ export const createParse = /* #__PURE__ */ factory(name, dependencies, ({
'%': 'mod',
mod: 'mod'
}

while (hasOwnProperty(operators, state.token)) {
name = state.token
fn = operators[name]
Expand Down

2 comments on commit 599f4ee

@zyw727725828
Copy link

@zyw727725828 zyw727725828 commented on 599f4ee May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

企业微信截图_17156716123063
% Has the priority order issue been fixed .I retested using this version and the calculation result is still wrong.

@josdejong
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation did not correspond to the actual behavior like discussed in #3189. That has been fixed in this commit.

The actual behavior itself is not "broken", though the precedence may be differing from what you want. On some points, different math applications use different rules for precedence. In case of doubt you can use parenthesis like (250/10) % 10.

Like I pointed out in #3189 (comment), if you want you can open a discussion to see if we should change this behavior.

Please sign in to comment.