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

parser, fmt: fix match with multi-commmented branchs (fix #19880) #19898

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

yuyi98
Copy link
Member

@yuyi98 yuyi98 commented Nov 16, 2023

This PR fix match with multi-commmented branchs (fix #19880).

  • Fix match with multi-commmented branchs.
  • Add test.

vlib\v\fmt\tests\match_with_multi_commented_branches_keep.vv

import strings

fn escape_string(s string) string {
	mut res := strings.new_builder(s.len * 2)
	for ch in s {
		match ch {
			0 // NUL (null)
			{
				res.write_u8(92) // \
				res.write_u8(48) // 0
			}
			10 { // LF (line feed)
				res.write_u8(92) // \
				res.write_u8(110) // n
			}
			13 { // CR (carriage return)
				res.write_u8(92) // \
				res.write_u8(114) // r
			}
			26 { // SUB (substitute)
				res.write_u8(92) // \
				res.write_u8(90) // Z
			}
			34,  // "
			39,  // '
			92 // \
			{
				res.write_u8(92) // \
				res.write_u8(ch)
			}
			else {
				res.write_u8(ch)
			}
		}
	}
	return res.bytestr()
}

fn main() {
}

@spytheman
Copy link
Member

Should vfmt support such comments @medvednikov ?

@JalonSolov
Copy link
Contributor

JalonSolov commented Nov 16, 2023

At first glance, they appear to be simple end-of-line comments, which should be supported.

However, this piece

			34,  // "
			39,  // '
			92 // \
			{

should be reformatted to

			34, 39, 92 { // ", ', \

or similar.

@medvednikov
Copy link
Member

I think this PR is fine.

@medvednikov
Copy link
Member

medvednikov commented Nov 17, 2023

34,  // "
39,  // '
92 // \

I think this is correct, and each branch should have its own comment, without merging them.

@spytheman spytheman merged commit 373da77 into vlang:master Nov 17, 2023
54 checks passed
@JalonSolov
Copy link
Contributor

The problem now is that v fmt has 2 different outputs, depending on whether or not comments are there.

If the original looks like

			34,  // "
			39,  // '
			92 // \
			{

and you run it through v fmt, it will still look the same after. However, if you have

			34,
			39,
			92
			{

and run it through v fmt, it will be reformatted to

                        34, 39, 92 {

Seems pretty inconsistent formatting, to me.

@medvednikov
Copy link
Member

I understand, I think it's fine.

It's the same with arrays for example

[1,2,3,
4,5,6]

will be vfmted to [1,2,3,4,5,6]

but if there are comments for each line, it'll be multiline

@yuyi98 yuyi98 deleted the fmt_match_branch branch November 19, 2023 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vfmt errors when formatting match statement with comments
4 participants