Skip to content
This repository has been archived by the owner on Dec 18, 2019. It is now read-only.

Literal: Support B'…' string format #47

Merged
merged 2 commits into from
Aug 9, 2016
Merged

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Aug 9, 2016

Fix #43.

Progression

  • A binary string starts with b but B is also valid and has the same effect,
  • A nowdoc string can be binary (with b or B).

A binary string starts with `b` but `B` is also valid and has the same
effect.
@Hywan Hywan merged commit d80e66e into tagua-vm:master Aug 9, 2016
@Hywan Hywan removed the in progress label Aug 9, 2016
@@ -239,7 +239,7 @@ fn string_single_quoted(input: &[u8]) -> Result<&[u8], Literal> {
return Result::Error(Error::Code(ErrorKind::Custom(StringError::TooShort as u32)));
}

if input[0] == 'b' as u8 {
if input[0] == 'b' as u8 || input[0] == 'B' as u8 {
Copy link

Choose a reason for hiding this comment

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

Wouldn't it be best to consolidate the binary string handling logic instead of having duplicated code between the single-quote and the nowdoc variants?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only this condition is similar. The minimum required heuristic length is different.
This is likely to change when we will support string interpolation (see #3).

Do you see any easy factorisation?

Copy link

Choose a reason for hiding this comment

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

Whoops! I just thought of chaining the check with the parser (binary_check -> string|nowdoc), but I didn't notice those were different. Sorry!

Copy link
Member Author

Choose a reason for hiding this comment

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

That's OK :-). Thanks for being attentive!

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

Successfully merging this pull request may close these issues.

2 participants