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

#44 Separate one-line header from next text block by paragraph-separate #45

Merged
merged 7 commits into from
Feb 1, 2024

Conversation

TobiasZawada
Copy link
Collaborator

Move one-line header regexp from adoc-re-paragraph-start to adoc-re-paragraph-separate.
paragraph-separate is merged into paragraph-start anyway.

This should fix #44.

@TobiasZawada TobiasZawada force-pushed the work/44SeparateHeaderFromTextParagraph branch from 760a76d to 497bb8b Compare January 30, 2024 02:33
@TobiasZawada
Copy link
Collaborator Author

TobiasZawada commented Jan 30, 2024

@bbatsov It is really a pity! cl-loop for int being the intervals of seems to be fundamentally broken in Emacs 28.1. One cannot get it working under both Emacs 28.1 AND 30.0.
In Emacs 28.1 there is an assignment to a free variable int without a preceding let (int) while in Emacs 30.0 there is an unused local lexical variable int with the preceding (let (int).

I even already tried a version-depending let-macro without success.

@TobiasZawada
Copy link
Collaborator Author

TobiasZawada commented Jan 30, 2024

@bbatsov Okay, the new macro adoc-cond-let does the trick, i.e., it fixes the problem for Emacs up to version 29.1 and leaves the code for Emacs 30.0 untouched up to a (let nil ,@body) which is essentially a (progn ,@body).

@bbatsov
Copy link
Owner

bbatsov commented Jan 31, 2024

I think it might be simpler to just avoid using cl-loop - it's a powerful macro, but few people are familiar with it and it tends to be a maintenance overhead in the long run.

@TobiasZawada
Copy link
Collaborator Author

TobiasZawada commented Jan 31, 2024

I think it might be simpler to just avoid using cl-loop - it's a powerful macro, but few people are familiar with it and it tends to be a maintenance overhead in the long run.

cl-loop is part of Emacs and it is not deprecated. Especially the for int being the intervals-feature is powerful. An alternative implementation would be quite messy.

In *scratch*-buffer:

(macroexpand '(cl-loop for int being the intervals property 'face
                   for pos = (car int)
                   for next = (cdr int)
                   for val = (get-text-property pos 'face)
                   when val do
                   (put-text-property
                    (+ start-src (1- pos)) (1- (+ start-src next)) 'face
                    val adoc-buffer)))
(cl--block-wrapper (catch '--cl-block-nil-- (let* ((pos nil) (next nil) (val nil) (--cl-var-- t)) (cl-block --cl-finish-- (cl--map-intervals (lambda (--cl-var1-- --cl-var2--) (setq int (cons --cl-var1-- --cl-var2--)) (setq pos (car int)) (setq next (cdr int)) (setq val (get-text-property pos 'face)) (if val (progn (put-text-property (+ start-src (1- pos)) (1- (+ start-src next)) 'face val adoc-buffer))) (setq --cl-var-- nil)) nil 'face nil nil)) nil)))

Therein, cl--map-intervals itself is also quite long and as internal function only usable from the cl-loop interface.

Note also that looks like the free-variable problem in cl-loop has been fixed in Emacs30.0.
Therefore, this adoc-cond-let is only needed for backward-compatibility, i.e., to avoid compilation warnings in earlier versions.

@bbatsov
Copy link
Owner

bbatsov commented Jan 31, 2024

I'm well aware of its status, but this was a contentious feature even in Common Lisp (where it originated), that's why I always avoid it. In general I always avoid cl-lib, as today there very few functions/macros there without alternatives in Emacs.

Anyways, if you feel very strongly about it I can live with its usage.

@TobiasZawada
Copy link
Collaborator Author

TobiasZawada commented Jan 31, 2024

The thing that makes a basic implementation of cl-loop for int being the intervals more complicated is that you only get the boundaries of text property changes with built-in functions like next-single-property-change and you have to construct the intervals by yourself -- together with all the pitfalls at the beginning and the end of the visible buffer. cl-loop for int being the intervals does that for you.

IMHO: If you exclude cl-lib you steel your own tools that are delivered by Emacs to you.

If you are curious you can have a look at the destructuring binding-capabilities of cl-loop for sexp = expr. I discovered these not long ago. For limited cases this is even simpler than the pcase construct.

https://ccrma.stanford.edu/CCRMA/Courses/AlgoComp/cm/doc/contrib/lispstyle.html
https://groups.google.com/g/comp.lang.lisp/c/Rc6xPNx9oR8/m/GAjOZf1Y8QwJ

A recursive search for cl-loop in the el.gz-files of /usr/share/emacs/29.1/lisp gives 772 hits.

@TobiasZawada
Copy link
Collaborator Author

@bbatsov Hope you are more comfortable with the latest version.
If so, please merge.

adoc-mode.el Outdated
(defun adoc-map-intervals (fun property &optional beg end object)
"Apply FUN to all intervals of PROPERTY in OBJECT in the region from BEG to END.
FUN is called with two arguments: the beginning and the end of the interval."
(unless object (setq object (current-buffer)))
Copy link
Owner

Choose a reason for hiding this comment

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

Why don't you just use let and something like (or beg (...)). Using setq to establish initial bindings is quite uncommon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are many techniques for assigning default arguments.
The unless ... setq-Method is one of the usual ones. Examples:

It works well since function arguments are lexically bound args and setq is a special form that can handle lexically bound args. The quote in (setq sym val) is not really evaluated in the sense of (set 'sym val) which couldn't handle lexically bound args well.

adoc-mode.el Outdated
(unless end (setq end (point-max)))
(let (end-interval)
(while
(progn
Copy link
Owner

Choose a reason for hiding this comment

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

progn is redundant in a while.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, that's your condition, not the body of the while. Might be good to move something in the body and just check for the final state here, as now it looks pretty weird.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is the usual loop with condition at the end.
Example: https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/jit-lock.el#n324

adoc-mode.el Outdated
(setq end-interval (next-single-property-change beg property object end))
(funcall fun beg end-interval)
(setq beg end-interval)
(null (= end-interval end))))))
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this null check needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If end is given as buffer position next-single-property-change returns that buffer position if no further property change is found. And that also indicates the last interval.

adoc-mode.el Outdated
val adoc-buffer)))
(adoc-map-intervals
(lambda (pos next)
(let ((val (get-text-property pos 'face)))
Copy link
Owner

Choose a reason for hiding this comment

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

There's also when-let that would simplify this code a bit.

Copy link
Collaborator Author

@TobiasZawada TobiasZawada Feb 1, 2024

Choose a reason for hiding this comment

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

I know when-let but I didn't know whether you like it.
It was long in subr-x.el instead of subr.el. Therefore, it was considered as seldom used feature:

commit b05a103ea7a26b2f4099a613015d9f1abdc39a4d
Author: Lars Ingebrigtsen larsi@gnus.org
Date: Sat Apr 30 16:42:44 2022 +0200

Move the when-let family of macros to subr.el

  • lisp/subr.el (internal--build-binding)
    (internal--build-bindings): Moved from subr-x.el and rewritten to
    not use the threading macro.
    (if-let*, when-let*, and-let*, if-let, when-let): Moved from
    subr-x.el. This avoids breaking the build every time somebody
    uses these macros in functions that end up being called during
    bootstrap.

@bbatsov
Copy link
Owner

bbatsov commented Feb 1, 2024

Won't in the simpler to convert your range to a list and map over it or use dolist?

I'm inclined to agree that the version with cl-loop was simpler than the current implementation, but I do think that map-interval can be simplified with my suggestion above.

@TobiasZawada
Copy link
Collaborator Author

TobiasZawada commented Feb 1, 2024

Won't in the simpler to convert your range to a list and map over it or use dolist?

I'm inclined to agree that the version with cl-loop was simpler than the current implementation, but I do think that map-interval can be simplified with my suggestion above.

I think that would even complicate things since it just adds one more structural element: an additional list.
The complicated thing is the construction of the intervals from their boudnaries.
This task remains if one collects the intervals in a list.

Or maybe, I misunderstood you.

@bbatsov
Copy link
Owner

bbatsov commented Feb 1, 2024

My point is that usually intervals/ranges are represented as lists in programming languages. That's why converting the boundaries to a list seems reasonable to me, but if you have concerns about this you ca just revert to the cl-loop solution.

@TobiasZawada
Copy link
Collaborator Author

TobiasZawada commented Feb 1, 2024

@bbatsov Instead of resetting the changes I really reverted them to keep the other version in the database.

I am merging this version now, since we already discussed it in some length.

@TobiasZawada TobiasZawada merged commit 0a189c5 into master Feb 1, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants