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

Emacs: Use built-in detection of comments #2040

Merged
merged 2 commits into from Sep 13, 2018

Conversation

Projects
None yet
4 participants
@Wilfred
Copy link
Contributor

commented Sep 11, 2018

This fixes https://caml.inria.fr/mantis/view.php?id=7844 where we crashed if we found text that looked like a definition inside a comment.

This is also faster and simpler. Emacs does handle nested comments correctly, but the comments worrying about this were written in 1996 when that may have been true.

Emacs: Use built-in detection of comments
This fixes
https://caml.inria.fr/mantis/view.php?id=7844
where we crashed if we found text that looked like a definition inside
a comment.

This is also faster and simpler. Emacs does handle nested comments
correctly, but the comments worrying about this were written in 1996
when that may have been true.
@gasche

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

Thanks!

I found (syntax-ppss) in the documentation, and indeed (nth 4 ...) is nil if not within a comment.

It would be nice to have someone that knows about the Emacs mode to give a look at this proposed patch. @Chris00, would you be able to comment on whether the change looks right to you? It would also affect Tuareg.

@@ -1137,57 +1097,7 @@ to the end.
(defun caml-in-comment-p ()
"Returns non-nil if point is inside a caml comment.
Returns nil for the parenthesis opening a comment."

This comment has been minimized.

Copy link
@gasche

gasche Sep 11, 2018

Member

What about this comment that the expected result on the opening parenthesis is nil, does the built-in Emacs function also respect that? (Otherwise I guess you could also check whether the position to the left is also within the comment.)

This comment has been minimized.

Copy link
@Chris00

Chris00 Sep 13, 2018

Member

(nth 4 (syntax-ppss)) returns nil on (* but considers *) as inside the comment.

This comment has been minimized.

Copy link
@gasche

gasche Sep 13, 2018

Member

So it works as the comment said it should, if I understand correctly.

This comment has been minimized.

Copy link
@Wilfred

Wilfred Sep 13, 2018

Author Contributor

Yep, that's correct. After this change, we still return nil when point (the cursor) is on the opening (.

@wyuenho

This comment has been minimized.

Copy link

commented Sep 12, 2018

There's one problem I found with this PR, prefixing a variable with in_ will mess up imenu as well.

module rec Foo : FOO = struct
  and in_hello =
    let b in
    in_hello (* error *)

Backtrace:

Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p nil)
  buffer-substring-no-properties(nil nil)
  caml-match-string(5)
  tuareg-imenu-create-index()
  imenu--make-index-alist()
  imenu-list-rescan-imenu()
  imenu-list-collect-entries()
  imenu-list-update()
  purpose-x-code1-update-changed()
  #f(compiled-function () #<bytecode 0x45089141>)()
  apply(#f(compiled-function () #<bytecode 0x45089141>) nil)
  timer-event-handler([t 0 1 0 nil #f(compiled-function () #<bytecode 0x45089141>) nil idle 0])

tuareg-imenu-create-index is just an alias to caml-create-index-function

@Chris00

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

prefixing a variable with in_ will mess up imenu as well.

I think this is unrelated — it happens without this modification as well.

@Chris00

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

Emacs does handle nested comments correctly,

Actually, (nth 4 (syntax-ppss)) returns the depth of the inner comment if it is nested.

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

Great! Thanks @Chris00 for the review. I will add a Changes entry and merge.

@gasche gasche merged commit f179a65 into ocaml:trunk Sep 13, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@Wilfred Wilfred deleted the Wilfred:fix-emacs-imenu branch Sep 13, 2018

@Wilfred

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

Thank you :)

If I have a chance, I will try to look at the in_ bug too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.