Skip to content

Decode \X escapes immediately#9245

Merged
faho merged 1 commit into
fish-shell:masterfrom
faho:decode-byte-escapes
Oct 5, 2022
Merged

Decode \X escapes immediately#9245
faho merged 1 commit into
fish-shell:masterfrom
faho:decode-byte-escapes

Conversation

@faho
Copy link
Copy Markdown
Member

@faho faho commented Sep 29, 2022

We forgot to decode (i.e. turn into nice wchar_t codepoints) "byte_literal" escape sequences. This meant that e.g.

string match ö \Xc3\Xb6

math 5 \X2b 5

didn't work, and the latter would print the wonderful error:

math: Error: Missing operator
'5 + 5'
   ^

In contrast, math 5 \x2b 5 works because \x is currently not "byte_literal" - it's seen as a codepoint number.

So, instead, we decode eagerly.

TODOs:

  • [N/A] Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

Sidenote: The diff is an absolute mess - what I did was:

  1. Pack the switch/case into a while loop so we can continue
  2. Add a std::string buffer for multiple \X escaped values
  3. Move an early if (errored) return none() into the loop
  4. Make the path for "byte_literal" (currently that's only \X) put the value into the buffer and continue if there's another \X following.

A reasonably simple change, but because of the additional indentation this touched most of read_unquoted_escape. Sorry about that!

We forgot to decode (i.e. turn into nice wchar_t codepoints)
"byte_literal" escape sequences. This meant that e.g.

```fish
string match ö \Xc3\Xb6

math 5 \X2b 5
```

didn't work, but `math 5 \x2b 5` did, and would print the wonderful
error:

```
math: Error: Missing operator
'5 + 5'
   ^
```

So, instead, we decode eagerly.
@faho faho mentioned this pull request Sep 29, 2022
3 tasks
Comment thread src/common.cpp
@faho faho added the bug Something that's not working as intended label Sep 30, 2022
@faho faho changed the title Decode multibyte escapes immediately Decode \X escapes immediately Oct 1, 2022
@faho faho added this to the fish 3.6.0 milestone Oct 5, 2022
@faho faho merged commit 396e276 into fish-shell:master Oct 5, 2022
@faho faho deleted the decode-byte-escapes branch October 5, 2022 16:55
@krobelus
Copy link
Copy Markdown
Contributor

krobelus commented Oct 6, 2022

the diff reads fine when ignoring whitespace

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented Oct 6, 2022

Just to share something that's helped me: I manually use difftastic from time-to-time for looking at patches like this, as it is AST-aware (or at least scope-aware) and can ignore superfluous changes like addition/removal of braces (when it doesn't change meaning), an if condition causing the entire resulting scope to be marked as changed, changes to hard-wrapped lines, etc.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something that's not working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants