Skip to content

Quoted extensions in comments, ocamllex and ocamlyacc#9166

Merged
Octachron merged 10 commits intoocaml:trunkfrom
314eter:quoted-extensions
Feb 3, 2020
Merged

Quoted extensions in comments, ocamllex and ocamlyacc#9166
Octachron merged 10 commits intoocaml:trunkfrom
314eter:quoted-extensions

Conversation

@314eter
Copy link
Copy Markdown
Contributor

@314eter 314eter commented Dec 6, 2019

As mentioned in #8820 (comment), quoted extensions should be handled in comments the same way quoted strings are.

I added support for quoted extensions in ocamllex and ocamlyacc too, but can move this to separate PR's if necessary.

The last commit fixes a bug introduced in #1932, that caused (* comments like this *) to be copied as (* c l t *).

Copy link
Copy Markdown
Contributor

@Drup Drup left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this looks great. The tests look pretty convincing, I just have small comments. There are also a bunch of lines that are over 80columns:

./lex/lexer.mll:296.81: [long-line] line is over 80 columns
./lex/lexer.mll:324.81: [long-line] line is over 80 columns

lex/lexer.mll Outdated
reset_string_buffer();
comment lexbuf }
| '{' (['a'-'z' '_'] * as delim) '|'
| '{' ('%' '%'? identstart identbody* ('.' identstart identbody*)* [' ' '\009' '\012']*)? (['a'-'z' '_']* as delim) "|"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you use/copy the regexes for identifier and whitespaces from the OCaml lexer instead ? It should use the exact same one, so that it's easier to check.

Copy link
Copy Markdown
Contributor

@Drup Drup left a comment

Choose a reason for hiding this comment

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

Excellent, let's merge when CI is happy.

@314eter
Copy link
Copy Markdown
Contributor Author

314eter commented Dec 9, 2019

Should I add a new changes entry, or add this PR number to the entry of #8820?

@gasche
Copy link
Copy Markdown
Member

gasche commented Dec 9, 2019

Adding to the existing entry would be fine, but then please add yourself as an extra author.

@Drup
Copy link
Copy Markdown
Contributor

Drup commented Jan 31, 2020

Somehow, this slipped between the cracks. Can we merge it now ? :)

@gasche
Copy link
Copy Markdown
Member

gasche commented Jan 31, 2020

I will leave this potential action in the hands of @Octachron (if it was me, I would want to have a quick look at the code again, and I don't have time right now; also I would wonder about backporting to 4.10 and it looks like a not-so-easy decision.)

@Octachron
Copy link
Copy Markdown
Member

Quoted extensions are a 4.11 feature, so there is no backporting decision to make here.

@Octachron
Copy link
Copy Markdown
Member

I reread the C part, it looks good now. @314eter , do you wish to clean-up the history before merging? Otherwise, I will squash all commits.

@314eter
Copy link
Copy Markdown
Contributor Author

314eter commented Feb 3, 2020

I don't have immediate access to my pc to clean up the history, so maybe squashing is easier.

@Octachron
Copy link
Copy Markdown
Member

As you wish, but there is no urgency (this is a bugfix for 4.11), so we can wait few weeks if you prefer.

@314eter
Copy link
Copy Markdown
Contributor Author

314eter commented Feb 3, 2020

You don't have to wait.

@Octachron Octachron merged commit 99224a9 into ocaml:trunk Feb 3, 2020
@Octachron
Copy link
Copy Markdown
Member

Merged, thanks!

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.

4 participants