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

Arithmetic expansion #714

Merged
merged 2 commits into from
May 24, 2020
Merged

Arithmetic expansion #714

merged 2 commits into from
May 24, 2020

Conversation

phy1729
Copy link
Member

@phy1729 phy1729 commented Mar 22, 2020

Needs docs and a changlog update (and perhaps a few more tests if any come to mind). One outstanding question is should "$(( ... ))" break the double quoted style as "$( ... )" does?

@danielshahaf danielshahaf added this to the 0.8.0 milestone Mar 22, 2020
@danielshahaf danielshahaf changed the title Arithmetic expn Arithmetic expansion Mar 22, 2020
@phy1729
Copy link
Member Author

phy1729 commented Mar 23, 2020

Added docs, changelog entry, and some more tests. Ready for review.

Outstanding questions:

  • Should "$(( ... ))" break the double quoted style as "$( ... )" does?
  • Which style should command substitution inside arithmetic substitution use: -quoted, -unquoted, or no suffix?

@phy1729 phy1729 force-pushed the arithmetic-expn branch 2 times, most recently from f1c7c80 to 996c657 Compare March 23, 2020 01:06
@danielshahaf
Copy link
Member

Needs docs and a changlog update (and perhaps a few more tests if any come to mind).

Thinking out loud:

  • Things that look like commands, but aren't:

    • $((telnet * 42)) (multiplication)
    • $((ls /etc)) (division)
    • $((diff - foo)) (subtraction)
    • $((zsh +1 -f /etc/passwd)) (addition, subtraction, and division)
    • $((ls & ls)) (bitwise and)
  • Things that look like arithmetic, but aren't:

    • $((ls ls)) (syntax error)
    • $(() function ()) (anonymous function)
    • $((echo case foo in bar\)); echo ';;') (subshell)
    • $((echo case foo in bar\)) (prefix of previous)
    • $((: ")) )(((" )) (what do you expect this to do, without running it? Arithmetic, subshell, or syntax error?)
    • $((: ") ))(((" )) (same question)

(I'm not saying that all of these be added; I'm just brainstorming.)

The rule is in lex.c.

Should "$(( ... ))" break the double quoted style as "$( ... )" does?

The criteria I would consider are:

  • Consistency across different types of quoting.

  • Forward compatibility with highlighting other forms of arithmetic expressions: e.g., subscripts, repeat loop counts, arithmetic for loops' control expressions.

  • Which approach would give theme authors more options. (E.g., if the possibilities with approach A are a strict superset of the possibilities with approach B, that'd be an argument in favour of the former.)

Which style should command substitution inside arithmetic substitution use: -quoted, -unquoted, or no suffix?

What kind of processing (word-splitting, etc) will it undergo? If it's the same processing in "$(foo)" and in this case, then it could use the same highlighting.

The same goes for $foo, by the way. In $(($foo + $bar)), both variables are double-quoted, right? So it'd be nice to use the double-quoted variables style (never mind that we don't have have a style for non-double-quoted variables).

Ready for review.

Ship it :-)

@danielshahaf
Copy link
Member

Triage: Ready to merge, and there's still enough time prior to tagging to find any issues: let's merge it! @phy1729, any reason not to? If not, would you do the honours?

@danielshahaf
Copy link
Member

Noting here that #709, once merged, would benefit from integration to this. (Though it's not clear yet which PR will be merged first.)

@phy1729 phy1729 merged commit 870bccf into zsh-users:master May 24, 2020
@phy1729
Copy link
Member Author

phy1729 commented May 24, 2020

Thanks for the reminder. Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants