[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Some problems in `add-log-current-defun'
|
From: |
Herbert Euler |
|
Subject: |
Re: Some problems in `add-log-current-defun' |
|
Date: |
Sun, 31 Dec 2006 16:28:14 +0800 |
PATCH FOR I, II, AND III
I have tested my approach, it works. The patch, which solves all of
I, II and III, is
*** add-log.el Thu Dec 28 20:49:44 2006
--- add-log.el.1-3 Sun Dec 31 11:33:21 2006
*************** (defun add-log-current-defun ()
*** 813,863 ****
(progn (forward-sexp 1)
(point))))
((memq major-mode add-log-c-like-modes)
! (beginning-of-line)
! ;; See if we are in the beginning part of a function,
! ;; before the open brace. If so, advance forward.
! (while (not (looking-at "{\\|\\(\\s *$\\)"))
! (forward-line 1))
! (or (eobp)
! (forward-char 1))
! (let (maybe-beg)
! ;; Try to find the containing defun.
! (beginning-of-defun)
! (end-of-defun)
! ;; If the defun we found ends before the desired
position,
! ;; see if there's a DEFUN construct
! ;; between that end and the desired position.
! (when (save-excursion
! (and (> location (point))
! (re-search-forward "^DEFUN"
! (save-excursion
! (goto-char location)
! (line-end-position))
! t)
! (re-search-forward "^{" nil t)
! (setq maybe-beg (point))))
! ;; If so, go to the end of that instead.
! (goto-char maybe-beg)
! (end-of-defun)))
! ;; If the desired position is within the defun we found,
! ;; find the function name.
! (when (< location (point))
! ;; Move back over function body.
! (backward-sexp 1)
! (let (beg)
! ;; Skip back over typedefs and arglist.
! ;; Stop at the function definition itself
! ;; or at the line that follows end of function doc
string.
! (forward-line -1)
! (while (and (not (bobp))
! (looking-at "[ \t\n]")
! (not (looking-back "[*]/)\n" (- (point)
4))))
! (forward-line -1))
! ;; If we found a doc string, this must be the DEFUN
macro
! ;; used in Emacs. Move back to the DEFUN line.
! (when (looking-back "[*]/)\n" (- (point) 4))
! (backward-sexp 1)
! (beginning-of-line))
;; Is this a DEFUN construct? And is LOCATION in it?
(if (and (looking-at "DEFUN\\b")
(>= location (point)))
--- 813,868 ----
(progn (forward-sexp 1)
(point))))
((memq major-mode add-log-c-like-modes)
! ;; See whether the point is inside a defun.
! (let* (having-previous-defun
! having-next-defun
! (previous-defun-end (save-excursion
! (setq having-previous-defun
! (c-beginning-of-defun))
! (c-end-of-defun)
! ;; `c-end-of-defun' move
! ;; point to the line
! ;; after the function
! ;; close, but the
! ;; position we prefer
! ;; here is the position
! ;; after the } that
! ;; closes the function.
! (backward-sexp 1)
! (forward-sexp 1)
! (point)))
! (next-defun-beginning (save-excursion
! (setq having-next-defun
! (c-end-of-defun))
! (c-beginning-of-defun)
! (point))))
! (if (and having-next-defun
! (< location next-defun-beginning))
! (skip-syntax-forward " "))
! (if (and having-previous-defun
! (> location previous-defun-end))
! (skip-syntax-backward " "))
! (unless (or
! ;; When there is no previous defun, the
! ;; point is not in a defun if it is not at
! ;; the beginning of the next defun.
! (and (not having-previous-defun)
! (not (= (point)
! next-defun-beginning)))
! ;; When there is no next defun, the point
! ;; is not in a defun if it is not at the
! ;; end of the previous defun.
! (and (not having-next-defun)
! (not (= (point)
! previous-defun-end)))
! ;; If the point is between two defuns, it
! ;; is not in a defun.
! (and (> (point) previous-defun-end)
! (< (point) next-defun-beginning)))
! ;; If the point is already at the beginning of a
! ;; defun, there is no need to move point again.
! (if (not (= (point) next-defun-beginning))
! (c-beginning-of-defun))
;; Is this a DEFUN construct? And is LOCATION in it?
(if (and (looking-at "DEFUN\\b")
(>= location (point)))
This should be more reliable than the previous implementation, since
it parses the file with the functions provided by CC mode, rather than
analysing text features in the file.
The basic idea of these changes is that, move the point to exactly the
same position where the previous implementation moves it to before
evaluating the forms that are not changed (i.e. forms starting from
line 861 in the orignal file, or the forms starting with ``(if (and
(looking-at "DEFUN\\b")'' in the orignal file) in a different way.
With the powerful functions provided by CC mode, the new
implementation is more reliable and cleaner.
There is a problem with this approach yet, because of a bug in CC
mode. The function `c-beginning-of-defun' cannot move point correctly
for functions like
int
main (argc, argv)
int argc;
char *argv[];
{
/* ... */
}
So `add-log-current-defun' gives wrong result for this kind of
functions. However, we can expect this problem to be fixed when the
bug is fixed in CC mode. Please see
(1) http://lists.gnu.org/archive/html/emacs-devel/2006-12/msg01341.html
for more information about this bug.
AGAIN, PATCH FOR IV AND ANOTHER PROBLEM
By the way, problem IV in the beginning of this thread can be applied
to some other code in `add-log-current-defun' too. That is, the
processing of `public', `private', `enum', `struct', `union' and
`class'. For instance, the name below is valid in C but the current
code cannot handle it correctly:
struct
abc {
};
When fixing it, I found that the patch I provided in link (2) to solve
C++ names problem caused another problem, keywords such as `struct' or
`enum' are not recorded when needed:
(2) http://lists.gnu.org/archive/html/emacs-devel/2006-12/msg01095.html
However, my later patch of IV in
(3) http://lists.gnu.org/archive/html/emacs-devel/2006-12/msg01226.html
eliminates the problem raised in applying the patch in link (2). So
the patch below, which fixes both the problem of `enum', `struct',
`union' and `class' (but not `public' and `private') and the problem
created by applying the patch in link (2), is based on the second
patch in link (3):
*** add-log.el Thu Dec 28 20:49:44 2006
--- add-log.el.4_2 Sun Dec 31 14:38:46 2006
*************** (defun add-log-current-defun ()
*** 916,941 ****
;; Include certain keywords if they
;; precede the name.
(setq middle (point))
! ;; Single (forward-sexp -1) invocation is
! ;; not enough for C++ member function defined
! ;; as part of nested class and/or namespace
! ;; like:
! ;;
! ;; void
! ;; foo::bar::baz::bazz ()
! ;; { ...
! ;;
! ;; Here we have to move the point to
! ;; the beginning of foo, not bazz.
! (while (not (looking-back "\\(^\\|[ \t]\\)"))
(forward-sexp -1))
;; Is this C++ method?
(when (and (< 2 middle)
! (string= (buffer-substring (-
middle 2)
! middle)
! "::"))
;; Include "classname::".
! (setq middle (point)))
;; Ignore these subparts of a class decl
;; and move back to the class name itself.
(while (looking-at "public \\|private ")
--- 916,939 ----
;; Include certain keywords if they
;; precede the name.
(setq middle (point))
! ;; Move through C++ nested name.
! (while (looking-back "::[ \t\n]*")
(forward-sexp -1))
+ (forward-sexp -1)
;; Is this C++ method?
(when (and (< 2 middle)
! (save-excursion
! (goto-char middle)
! (looking-back "::[ \t\n]*")))
;; Include "classname::".
! (save-excursion
! ;; The `forward-sexp' form after
! ;; the `while' form above moves
! ;; backward one more sexp, so we
! ;; move forward one.
! (forward-sexp 1)
! (re-search-forward "\\(\\s \\|\n\\)*")
! (setq middle (point))))
;; Ignore these subparts of a class decl
;; and move back to the class name itself.
(while (looking-at "public \\|private ")
*************** (defun add-log-current-defun ()
*** 944,960 ****
(backward-sexp 1)
(setq middle (point))
(forward-word -1))
! (and (bolp)
! (looking-at
! "enum \\|struct \\|union \\|class ")
! (setq middle (point)))
(goto-char end)
(when (eq (preceding-char) ?=)
(forward-char -1)
(skip-chars-backward " \t")
(setq end (point)))
! (buffer-substring-no-properties
! middle end))))))))
((memq major-mode add-log-tex-like-modes)
(if (re-search-backward
"\\\\\\(sub\\)*\\(section\\|paragraph\\|chapter\\)"
--- 942,969 ----
(backward-sexp 1)
(setq middle (point))
(forward-word -1))
! (and
! ;; These defuns are not necessary
! ;; starting at column 0.
! ;; (bolp)
! (looking-at
! "\\(enum\\|struct\\|union\\|class\\)[
\t\n]")
! (setq middle (point)))
(goto-char end)
(when (eq (preceding-char) ?=)
(forward-char -1)
(skip-chars-backward " \t")
(setq end (point)))
! (let ((name (buffer-substring-no-properties
! middle end)))
! (setq name (replace-regexp-in-string
! "\n" " " name)
! name (replace-regexp-in-string
!
"\\(enum\\|struct\\|union\\|class\\)[ \t]+"
! "\\1 "
! name)
! name (replace-regexp-in-string
! "[ \t]*::[ \t]*" "::"
name))))))))))
((memq major-mode add-log-tex-like-modes)
(if (re-search-backward
"\\\\\\(sub\\)*\\(section\\|paragraph\\|chapter\\)"
These changes are guaranteed not to cause other problems during my
test.
PATCH FOR `public' AND `private'
The reason of separating the patch for `public' and `private' is that
it is different from other patches. The body of the `while' form that
contains `public' and `private' is really the same as the forms before
the while form, so it should be a whole iteration, not a new one. So
the patch is based on the file with the patch in the above section:
*** add-log.el.4_2 Sun Dec 31 14:38:46 2006
--- add-log.el.other Sun Dec 31 15:17:03 2006
*************** (defun add-log-current-defun ()
*** 906,947 ****
;; Consistency check: going down and up
;; shouldn't take us back before BEG.
(> (point) beg))
! (let (end middle)
! ;; Don't include any final whitespace
! ;; in the name we use.
! (skip-chars-backward " \t\n")
! (setq end (point))
! (backward-sexp 1)
! ;; Now find the right beginning of the name.
! ;; Include certain keywords if they
! ;; precede the name.
! (setq middle (point))
! ;; Move through C++ nested name.
! (while (looking-back "::[ \t\n]*")
! (forward-sexp -1))
! (forward-sexp -1)
! ;; Is this C++ method?
! (when (and (< 2 middle)
! (save-excursion
! (goto-char middle)
! (looking-back "::[ \t\n]*")))
! ;; Include "classname::".
! (save-excursion
! ;; The `forward-sexp' form after
! ;; the `while' form above moves
! ;; backward one more sexp, so we
! ;; move forward one.
! (forward-sexp 1)
! (re-search-forward "\\(\\s \\|\n\\)*")
! (setq middle (point))))
! ;; Ignore these subparts of a class decl
! ;; and move back to the class name itself.
! (while (looking-at "public \\|private ")
! (skip-chars-backward " \t:")
(setq end (point))
(backward-sexp 1)
(setq middle (point))
! (forward-word -1))
(and
;; These defuns are not necessary
;; starting at column 0.
--- 906,944 ----
;; Consistency check: going down and up
;; shouldn't take us back before BEG.
(> (point) beg))
! (let (end
! middle
! (first-iter t))
! (while (or first-iter
! (looking-at
!
"\\(public\\|protected\\|private\\)[ \t\n]"))
! (setq first-iter nil)
! ;; Skip characters that are not for
identifiers.
! (skip-chars-backward ": \t\n")
(setq end (point))
(backward-sexp 1)
+ ;; Now find the right beginning of the
name.
+ ;; Include certain keywords if they
+ ;; precede the name.
(setq middle (point))
! ;; Move through C++ nested name.
! (while (looking-back "::[ \t\n]*")
! (forward-sexp -1))
! (forward-sexp -1)
! ;; Is this C++ method?
! (when (and (< 2 middle)
! (save-excursion
! (goto-char middle)
! (looking-back "::[ \t\n]*")))
! ;; Include "classname::".
! (save-excursion
! ;; The `forward-sexp' form after
! ;; the `while' form above moves
! ;; backward one more sexp, so we
! ;; move forward one.
! (forward-sexp 1)
! (re-search-forward "\\(\\s \\|\n\\)*")
! (setq middle (point)))))
(and
;; These defuns are not necessary
;; starting at column 0.
Note that `protected' is added here.
PUTTING TOGETHER
Finally I want to put all changes together, since there are too many
changes to make really a new implementation. The patch for solving
all of I, II, III, and IV is
*** add-log.el Thu Dec 28 20:49:44 2006
--- add-log.el.new Sun Dec 31 16:13:48 2006
*************** (defun add-log-current-defun ()
*** 813,863 ****
(progn (forward-sexp 1)
(point))))
((memq major-mode add-log-c-like-modes)
! (beginning-of-line)
! ;; See if we are in the beginning part of a function,
! ;; before the open brace. If so, advance forward.
! (while (not (looking-at "{\\|\\(\\s *$\\)"))
! (forward-line 1))
! (or (eobp)
! (forward-char 1))
! (let (maybe-beg)
! ;; Try to find the containing defun.
! (beginning-of-defun)
! (end-of-defun)
! ;; If the defun we found ends before the desired
position,
! ;; see if there's a DEFUN construct
! ;; between that end and the desired position.
! (when (save-excursion
! (and (> location (point))
! (re-search-forward "^DEFUN"
! (save-excursion
! (goto-char location)
! (line-end-position))
! t)
! (re-search-forward "^{" nil t)
! (setq maybe-beg (point))))
! ;; If so, go to the end of that instead.
! (goto-char maybe-beg)
! (end-of-defun)))
! ;; If the desired position is within the defun we found,
! ;; find the function name.
! (when (< location (point))
! ;; Move back over function body.
! (backward-sexp 1)
! (let (beg)
! ;; Skip back over typedefs and arglist.
! ;; Stop at the function definition itself
! ;; or at the line that follows end of function doc
string.
! (forward-line -1)
! (while (and (not (bobp))
! (looking-at "[ \t\n]")
! (not (looking-back "[*]/)\n" (- (point)
4))))
! (forward-line -1))
! ;; If we found a doc string, this must be the DEFUN
macro
! ;; used in Emacs. Move back to the DEFUN line.
! (when (looking-back "[*]/)\n" (- (point) 4))
! (backward-sexp 1)
! (beginning-of-line))
;; Is this a DEFUN construct? And is LOCATION in it?
(if (and (looking-at "DEFUN\\b")
(>= location (point)))
--- 813,868 ----
(progn (forward-sexp 1)
(point))))
((memq major-mode add-log-c-like-modes)
! ;; See whether the point is inside a defun.
! (let* (having-previous-defun
! having-next-defun
! (previous-defun-end (save-excursion
! (setq having-previous-defun
! (c-beginning-of-defun))
! (c-end-of-defun)
! ;; `c-end-of-defun' move
! ;; point to the line
! ;; after the function
! ;; close, but the
! ;; position we prefer
! ;; here is the position
! ;; after the } that
! ;; closes the function.
! (backward-sexp 1)
! (forward-sexp 1)
! (point)))
! (next-defun-beginning (save-excursion
! (setq having-next-defun
! (c-end-of-defun))
! (c-beginning-of-defun)
! (point))))
! (if (and having-next-defun
! (< location next-defun-beginning))
! (skip-syntax-forward " "))
! (if (and having-previous-defun
! (> location previous-defun-end))
! (skip-syntax-backward " "))
! (unless (or
! ;; When there is no previous defun, the
! ;; point is not in a defun if it is not at
! ;; the beginning of the next defun.
! (and (not having-previous-defun)
! (not (= (point)
! next-defun-beginning)))
! ;; When there is no next defun, the point
! ;; is not in a defun if it is not at the
! ;; end of the previous defun.
! (and (not having-next-defun)
! (not (= (point)
! previous-defun-end)))
! ;; If the point is between two defuns, it
! ;; is not in a defun.
! (and (> (point) previous-defun-end)
! (< (point) next-defun-beginning)))
! ;; If the point is already at the beginning of a
! ;; defun, there is no need to move point again.
! (if (not (= (point) next-defun-beginning))
! (c-beginning-of-defun))
;; Is this a DEFUN construct? And is LOCATION in it?
(if (and (looking-at "DEFUN\\b")
(>= location (point)))
*************** (defun add-log-current-defun ()
*** 906,960 ****
;; Consistency check: going down and up
;; shouldn't take us back before BEG.
(> (point) beg))
! (let (end middle)
! ;; Don't include any final whitespace
! ;; in the name we use.
! (skip-chars-backward " \t\n")
! (setq end (point))
! (backward-sexp 1)
! ;; Now find the right beginning of the name.
! ;; Include certain keywords if they
! ;; precede the name.
! (setq middle (point))
! ;; Single (forward-sexp -1) invocation is
! ;; not enough for C++ member function defined
! ;; as part of nested class and/or namespace
! ;; like:
! ;;
! ;; void
! ;; foo::bar::baz::bazz ()
! ;; { ...
! ;;
! ;; Here we have to move the point to
! ;; the beginning of foo, not bazz.
! (while (not (looking-back "\\(^\\|[ \t]\\)"))
! (forward-sexp -1))
! ;; Is this C++ method?
! (when (and (< 2 middle)
! (string= (buffer-substring (-
middle 2)
! middle)
! "::"))
! ;; Include "classname::".
! (setq middle (point)))
! ;; Ignore these subparts of a class decl
! ;; and move back to the class name itself.
! (while (looking-at "public \\|private ")
! (skip-chars-backward " \t:")
(setq end (point))
(backward-sexp 1)
(setq middle (point))
! (forward-word -1))
! (and (bolp)
! (looking-at
! "enum \\|struct \\|union \\|class ")
! (setq middle (point)))
(goto-char end)
(when (eq (preceding-char) ?=)
(forward-char -1)
(skip-chars-backward " \t")
(setq end (point)))
! (buffer-substring-no-properties
! middle end))))))))
((memq major-mode add-log-tex-like-modes)
(if (re-search-backward
"\\\\\\(sub\\)*\\(section\\|paragraph\\|chapter\\)"
--- 911,971 ----
;; Consistency check: going down and up
;; shouldn't take us back before BEG.
(> (point) beg))
! (let (end
! middle
! (first-iter t))
! (while (or first-iter
! (looking-at
!
"\\(public\\|protected\\|private\\)[ \t\n]"))
! (setq first-iter nil)
! ;; Skip characters that are not for
identifiers.
! (skip-chars-backward ": \t\n")
(setq end (point))
(backward-sexp 1)
+ ;; Now find the right beginning of the
name.
+ ;; Include certain keywords if they
+ ;; precede the name.
(setq middle (point))
! ;; Move through C++ nested name.
! (while (looking-back "::[ \t\n]*")
! (forward-sexp -1))
! (forward-sexp -1)
! ;; Is this C++ method?
! (when (and (< 2 middle)
! (save-excursion
! (goto-char middle)
! (looking-back "::[ \t\n]*")))
! ;; Include "classname::".
! (save-excursion
! ;; The `forward-sexp' form after
! ;; the `while' form above moves
! ;; backward one more sexp, so we
! ;; move forward one.
! (forward-sexp 1)
! (re-search-forward "\\(\\s \\|\n\\)*")
! (setq middle (point)))))
! (and
! ;; These defuns are not necessary
! ;; starting at column 0.
! ;; (bolp)
! (looking-at
! "\\(enum\\|struct\\|union\\|class\\)[
\t\n]")
! (setq middle (point)))
(goto-char end)
(when (eq (preceding-char) ?=)
(forward-char -1)
(skip-chars-backward " \t")
(setq end (point)))
! (let ((name (buffer-substring-no-properties
! middle end)))
! (setq name (replace-regexp-in-string
! "\n" " " name)
! name (replace-regexp-in-string
!
"\\(enum\\|struct\\|union\\|class\\)[ \t]+"
! "\\1 "
! name)
! name (replace-regexp-in-string
! "[ \t]*::[ \t]*" "::"
name))))))))))
((memq major-mode add-log-tex-like-modes)
(if (re-search-backward
"\\\\\\(sub\\)*\\(section\\|paragraph\\|chapter\\)"
And I will provide many test cases. The all-together patch and the
test cases are attached. Most of the cases are passed by the new
implementation. Of course, there are two cases can't be passed. We
can test it again when Alan fix the bug in CC mode (See link (1)).
From these cases, I can conclude that these changes (i.e. the new
implementation) do not break anything else, and can behave for ``bad
formatted'' or ``not reasonably formatted'' code better than the
previous implementation. It is cleaner and more reliable than the
previous one.
Regards,
Guanpeng Xu
From: Richard Stallman <address@hidden>
Reply-To: address@hidden
To: "Herbert Euler" <address@hidden>
CC: address@hidden
Subject: Re: Some problems in `add-log-current-defun'
Date: Fri, 29 Dec 2006 10:44:33 -0500
I have solved I and IV, more or less.
We want to fix I and II and maybe III.
But not necessarily IV, not if it makes the change more
complex or risks breaking something else.
_________________________________________________________________
Don't just search. Find. Check out the new MSN Search!
http://search.msn.click-url.com/go/onm00200636ave/direct/01/
add-log.el.patch
Description: Binary data
add_log_test.c
Description: Binary data
Re: Some problems in `add-log-current-defun',
Herbert Euler <=