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

swiper.el: Don't highlight multiline matches #1600

Closed

Conversation

raxod502
Copy link
Contributor

@raxod502 raxod502 commented May 27, 2018

This corrects an inconsistency between the search results and the highlighting which happens when a regexp can match newlines.

This corrects an inconsistency between the search results and the
highlighting which happens when a regexp can match newlines.
swiper.el Outdated
swiper-faces)
wnd i)
(cl-incf i)))))))))))
;; Don't highlight a match if it goes across
Copy link
Collaborator

@basil-conto basil-conto May 27, 2018

Choose a reason for hiding this comment

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

Nitpick: how about "spans" instead of "goes across"?

Copy link
Contributor Author

@raxod502 raxod502 May 27, 2018

Choose a reason for hiding this comment

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

Fixed.

swiper.el Outdated
;; Don't highlight a match if it goes across
;; multiple lines.
(unless (> (line-number-at-pos (match-end 0))
(line-number-at-pos (match-beginning 0)))
Copy link
Collaborator

@basil-conto basil-conto May 27, 2018

Choose a reason for hiding this comment

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

Why not

(when (> 2 (count-lines (match-beginning 0) (match-end 0))) ...)

or similar?

Copy link
Contributor Author

@raxod502 raxod502 May 27, 2018

Choose a reason for hiding this comment

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

I didn't know about count-lines. But that actually seems less clear to me. One has to figure out why 2 instead of 1, and the docstring does not seem all too obvious to me:

Return number of lines between START and END.
This is usually the number of newlines between them,
but can be one more if START is not equal to END
and the greater of them is not at the start of a line.

I think using line-number-at-pos makes it obvious that what we are checking is if the beginning and ending are on different lines.

Copy link
Contributor Author

@raxod502 raxod502 May 27, 2018

Choose a reason for hiding this comment

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

Actually, line-number-at-pos is super slow, since it recomputes the line number from the beginning of the buffer. Checking if the match has a newline should be faster.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented May 27, 2018

But that actually seems less clear to me. One has to figure out why 2 instead of 1, and the docstring does not seem all too obvious to me

In these cases it is customary to

  • add a comment explaining the logic and/or write a helper function with a clearer docstring
  • look up count-lines in the Elisp manual, e.g. via C-hScount-linesRET, where the return value is explained in greater detail
  • report-emacs-bug suggesting to clarify the docstring of count-lines

Actually, line-number-at-pos is super slow, since it recomputes the line number from the beginning of the buffer.

Yes, which is why I suggested calling count-lines directly.

Checking if the match has a newline should be faster.

This is also a possibility, but are you sure that:

  1. constructing a match-string is faster than count-lines (the latter is backed by a fast newline cache)
  2. checking for carriage returns, as count-lines does when selective-display is enabled, is never required

?

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented May 27, 2018

are you sure that [...] constructing a match-string is faster than count-lines (the latter is backed by a fast newline cache) [...] ?

FWIW, here's a toy benchmark suggesting allocating strings is only a tiny bit slower, but uses a lot more memory:

(defun my-a ()
  (string-match-p "\n" (match-string 0)))

(defun my-b ()
  (<= 2 (count-lines (match-beginning 0) (match-end 0))))

(defun my-c ()
  (save-excursion (search-backward "\n" (match-beginning 0) t)))

(defun my-run (pred)
  (goto-char (point-min))
  (let (matches)
    (while (re-search-forward (rx "sit" (+? anything) "tellus") nil t)
      (when (funcall pred)
        (push (point) matches)))
    matches))

(byte-compile #'my-run)

(with-temp-buffer
  (dotimes (_ 500)
    (insert "\
Lorem ipsum dolor sit amet, consectetuer adipiscing elit.
Donec hendrerit tempor tellus.
Proin quam nisl, tincidunt et, mattis eget, convallis nec, purus.\n"))
  (let ((fns '(my-c my-b my-a)))
    ;; Check equivalence of predicates.
    (cl-assert (= 1 (length (delete-dups (mapcar #'my-run fns)))))
    (dolist (fn fns)
      (byte-compile fn)
      (garbage-collect)
      (message "%s: %s" fn (benchmark-run 1000 (my-run fn))))))

The result is:

my-c: (2.370037653 35 0.23561634299999995)
my-b: (2.421288264 35 0.23625290000000015)
my-a: (2.805098317 80 0.5420247640000002)

Premature optimisation FTW! :)

@raxod502
Copy link
Contributor Author

@raxod502 raxod502 commented May 27, 2018

OK, I have no real objection to using count-lines, so I changed it to that.

@alphapapa
Copy link

@alphapapa alphapapa commented Jun 3, 2018

Thanks to both of you, as this was very instructive. I didn't know about count-lines either, or that it has a cache. May I suggest a blog article? =)

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Jun 3, 2018

I didn't know about count-lines either, or that it has a cache.

It's not count-lines that has the cache per se, but each buffer object. See, for example, function newline-cache-check. count-lines uses forward-line, which calls find_newline, which in turn uses the buffer's newline_cache.

For some recent discussion about these things (which is how I found out about them), see the emacs-devel thread "State of the overlay tree branch" from March 2018, particularly the following messages:

May I suggest a blog article?

You may, but I don't blog; perhaps someone else will want to.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jun 14, 2018

Thanks.

@raxod502 raxod502 deleted the feat/disallow-multiline-highlighting branch Jun 14, 2018
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.

None yet

4 participants