Skip to content

Octal character literals and identifiers in comments#4

Merged
Chris00 merged 2 commits intoocaml:masterfrom
314eter:master
Apr 12, 2019
Merged

Octal character literals and identifiers in comments#4
Chris00 merged 2 commits intoocaml:masterfrom
314eter:master

Conversation

@314eter
Copy link
Copy Markdown
Contributor

@314eter 314eter commented Feb 26, 2019

This is a companion PR for ocaml/ocaml#1901.

; note: this is only to go faster than one character at a time
(defconst caml-font-other-comment-re
"[^{(*\"'\012\015]+"
"[^A-Za-z_\300-\326\330-\366\370-\377{(*\"'\012\015]+"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do not understand: AZ are not special, why are they excluded (^)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Otherwise in (* f' '"' *) the f will be matched as caml-font-other-comment-re, then ' ' as a character, and the rest as an unclosed string. With this change, f' is matched as caml-font-ident-re and '"' as a character.

((looking-at caml-font-newline-re)
(goto-char (match-end 0))
(setq continue (caml-font-put-state (match-end 0) (cons nil depth))))
((caml-font-looking-at caml-font-ident-re)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this useful? Won't the caml-font-looking-at caml-font-ident-or-num-re above already catch this case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The caml-font-ident-or-num-re above is in another state (outside comments).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks — did not see that when glancing at it in github.

@Chris00 Chris00 merged commit 5569e23 into ocaml:master Apr 12, 2019
@314eter
Copy link
Copy Markdown
Contributor Author

314eter commented Apr 12, 2019

Maybe it was a bit early to merge this since ocaml/ocaml#1901 is not merged yet.

@Chris00
Copy link
Copy Markdown
Member

Chris00 commented Apr 12, 2019

Well, the outlook seems rather positive — and Tuareg already handles it the same way! ☺
If it ends up not being merged, we'll revert this change — and I will update Tuareg as well.

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.

2 participants