unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Some problems in `add-log-current-defun'
@ 2006-12-27 10:32 Herbert Euler
  2006-12-27 10:45 ` David Kastrup
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Herbert Euler @ 2006-12-27 10:32 UTC (permalink / raw)


I encountered a problem in add-log.el, and solved it somehow (see
http://lists.gnu.org/archive/html/emacs-devel/2006-09/msg00869.html).
However, after re-reading the implementation of
`add-log-current-defun', I found there are still many problems (bugs)
in `add-log-current-defun' for the C like languages.  I'm going to
work on it, but before doing anything, I'd like to write them down.
This message is intended to describe the problems I found, rather than
talking about any proposal.  Please help me find whether there are
more problems not found yet.

Please locate to the function `add-log-current-defun' in `add-log.el'
before continuing reading.

I. The end of a function

The point is moved to the start position of a function or an empty
line (lines consist of only white space characters) with the following
code.

    ;; 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))

But this is not reliable.  If someone forgets to put a newline after a
function, `add-log-current-defun' will report wrong name.  Please
consider the following example:

    int
    f1 ()
    {
      /* If point is here `add-log-current-defun' gets wrong result.  */
    }
    int
    f2 ()
    {
      /* ...  */
    }

When the point is inside the body of `f1', invoking
`add-log-current-defun' will get `f2', rather than `f1'.

II. On the change of `beginning-of-defun' in CC mode

When the point is in the docstring of Emacs C source code, the
following forms are evaluated.

    (let (maybe-beg)
      ;; Try to find the containing defun.
      (beginning-of-defun)
      (end-of-defun)

But I found the result is wrong with the newest CC mode.  Consider the
following Emacs C source code:

    DEFUN ("catch", Fcatch, Scatch, 1, UNEVALLED, 0,
           doc: /* Eval BODY allowing nonlocal exits using `throw'.
    TAG is evalled to get the tag to use; it must not be nil.

    Then the BODY is executed.
    Within BODY, a call to `throw' with the same TAG exits BODY and this 
`catch'.
    If no throw happens, `catch' returns the value of the last BODY form.
    If a throw happens, it specifies the value to return from `catch'.
    usage: (catch TAG BODY...)  */)

Now suppose the point is at the beginning of the second paragarph,
i.e. before ``Then''.  This is where the point will be before
evaluating the forms given above if one invokes
`add-log-current-defun' when the point is in the first paragraph of
the docstring.  In the past, CC mode does not consider the arguments
of DEFUN as a defun, so `beginning-of-defun' will move point to the
beginning of the function that appear before this DEFUN.  With the
forms in `add-log-current-defun', the result is correct.  But I found
in the newest CC mode considers the arguments (starting with
``("catch"'', ending with ``*/)'') as a defun, so that
`beginning-of-defun' will move point to the beginning of the
arguments, i.e. between the space that is after ``DEFUN'' and the left
paren before ``"catch"''.  As a result, one cannot produce correct
change log entry when point is in the first paragraph of this
function, for example, when point is between ``Eval'' and ``BODY'' in
the first paragraph.

III. Different styles

The function skips typedefs and arglist with the following forms.

        ;; 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))

This is not general: it cannot process programs in some style.  In
section 7.7 of the 3rd edition of The C++ Programming Language by
Bjarne Stroustrup, there is a shell sort implementation:

    void ssort(void * base, size_t n, size_t sz, CFT cmp)
    /*
        Sort the "n" elements of vector "base" into increasing order
        using the comparison function pointed to by "cmp".
        The elements are of size "sz".

        Shell sort (Knuth, Vol3, pg84)
    */
    {
        /* ...  */
    }

The current implementation cannot handle programs in this style
correctly.

IV. On the C++ names

And what I tried to fix is not general too.  My fix is

    (while (not (looking-back "\\(^\\|[ \t]\\)"))
      (forward-sexp -1))

This is not general too: C++ permits the nested name to be put in many
lines.  For example, the following name is valid:

    void
    class_1
    ::
    sub_class_2
    ::
    method_3 ()
    {
      /* ...  */
    }

The current implementation cannot handle this name correctly.

Regards,
Guanpeng Xu

_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE! 
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: Some problems in `add-log-current-defun'
@ 2006-12-31  8:28 Herbert Euler
  2006-12-31 22:13 ` Richard Stallman
  0 siblings, 1 reply; 19+ messages in thread
From: Herbert Euler @ 2006-12-31  8:28 UTC (permalink / raw)
  Cc: emacs-devel

[-- Attachment #1: Type: text/plain, Size: 35028 bytes --]

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 <rms@gnu.org>
>Reply-To: rms@gnu.org
>To: "Herbert Euler" <herberteuler@hotmail.com>
>CC: emacs-devel@gnu.org
>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/

[-- Attachment #2: add-log.el.patch --]
[-- Type: application/octet-stream, Size: 8463 bytes --]

*** 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\\)"

[-- Attachment #3: add_log_test.c --]
[-- Type: application/octet-stream, Size: 5047 bytes --]


/* 1. K&R C.  */

/* This will not get correct result until the bug in CC mode is fixed.
   See
   http://lists.gnu.org/archive/html/emacs-devel/2006-12/msg01341.html  */
int
main1 (argc, argv)
     int argc;
     char *argv[];
{
  /* ...  */
}

int
f1 (arg)
     Lisp_Object arg;
{
}

int
f2 (arg)
     Lisp_Object arg;
     /* The `arg' is a Lisp_Object.  */
{
}

DEFUN ("catch", Fcatch1, Scatch, 1, UNEVALLED, 0,
       doc: /* Eval BODY allowing nonlocal exits using `throw'.
TAG is evalled to get the tag to use; it must not be nil.

Then the BODY is executed.
Within BODY, a call to `throw' with the same TAG exits BODY and this `catch'.
If no throw happens, `catch' returns the value of the last BODY form.
If a throw happens, it specifies the value to return from `catch'.
usage: (catch TAG BODY...)  */)
     (args)
     Lisp_Object args;
{
  register Lisp_Object tag;
  struct gcpro gcpro1;

  GCPRO1 (args);
  tag = Feval (Fcar (args));
  UNGCPRO;
  return internal_catch (tag, Fprogn, Fcdr (args));
}

/* Now, some badly formatted code.  */

   int
 main2 (argc, argv)
      int argc;
      char *argv[];
   {
                 /* ...  */
 }

                        int
                                          f3
         (arg)
                  Lisp_Object

           arg


;
     {
}

int                            f4
                (arg)
               Lisp_Object arg;
     /* The `arg' is a Lisp_Object.  */
  {
          }

                      DEFUN ("catch", Fcatch2, Scatch, 1, UNEVALLED, 0,
         doc: /* Eval BODY allowing nonlocal exits using `throw'.
       TAG is evalled to get the tag to use; it must not be nil.

Then the BODY is executed.
Within BODY, a call to `throw' with the same TAG exits BODY and this `catch'.
If no throw happens, `catch' returns the value of the last BODY form.
If a throw happens, it specifies the value to return from `catch'.
usage: (catch TAG BODY...)  */)
     (args)
     Lisp_Object args;
{
  register Lisp_Object tag;
  struct gcpro gcpro1;

  GCPRO1 (args);
                               tag = Feval (Fcar (args));
  UNGCPRO;
           return internal_catch (tag, Fprogn, Fcdr (args));
     }

/* 2. ANSI/ISO C.  */

int
main3 (int argc, char *argv[])
{
}

int
f5 (Lisp_Object arg)
{
}

int
f6 (Lisp_Object arg)
/* The `arg' is a Lisp_Object.  */
{
}

DEFUN ("catch", Fcatch3, Scatch, 1, UNEVALLED, 0,
       doc: /* Eval BODY allowing nonlocal exits using `throw'.
TAG is evalled to get the tag to use; it must not be nil.

Then the BODY is executed.
Within BODY, a call to `throw' with the same TAG exits BODY and this `catch'.
If no throw happens, `catch' returns the value of the last BODY form.
If a throw happens, it specifies the value to return from `catch'.
usage: (catch TAG BODY...)  */)
     (Lisp_Object args)
{
  register Lisp_Object tag;
  struct gcpro gcpro1;

  GCPRO1 (args);
  tag = Feval (Fcar (args));
  UNGCPRO;
  return internal_catch (tag, Fprogn, Fcdr (args));
}

/* Now, some badly formatted code.  */

        int

             main4

 (int
argc,
char *
argv[])
   {
       }

                  int
f7
 (
Lisp_Object
                            arg)
         {
                       }

                        int

        f8

(Lisp_Object
arg)
/* The `arg' is a Lisp_Object.  */
{
}

                        DEFUN ("catch", Fcatch4, Scatch, 1, UNEVALLED, 0,
       doc: /* Eval BODY allowing nonlocal exits using `throw'.
TAG is evalled to get the tag to use; it must not be nil.

Then the BODY is executed.
Within BODY, a call to `throw' with the same TAG exits BODY and this `catch'.
If no throw happens, `catch' returns the value of the last BODY form.
If a throw happens, it specifies the value to return from `catch'.
usage: (catch TAG BODY...)  */)
     (Lisp_Object args)
{
  register Lisp_Object tag;
  struct gcpro gcpro1;

  GCPRO1 (args);
  tag = Feval (Fcar (args));
  UNGCPRO;
  return internal_catch (tag, Fprogn, Fcdr (args));
}

/* 3. C++ functions.  */

int
a_b::c_d::e_f1 ()
{
}

int
a_b
::
c_d::e_f2 ()
{
}

int
  a_b
    ::
      c_d
        ::
          e_f3
            ()
{
}

/* 4. struct, enum, class, union; private, protected, public.  */

/* The cases of `struct' can represent all `struct', `enum', `class',
   and `union'.  */

struct struct1
{
};

struct
struct2
{
};

   struct struct3
{
};

   struct
struct4
{
};

class a_b1 : public c_d
{
};

class a_b2
: public c_d
{
};

class a_b3:
public c_d
{
};

class a_b4
:
public c_d
{
};

    class
a_b5 : public c_d
{
};

 class a_b6
    : public
                   c_d
{
};

 class
 a_b7           :
      public c_d
{
};

   class
a_b8
 :
          public
c_d
{
};

class a_b::c_d1 : public e_f, public g_h 
{
};

class a_b
::
c_d2 
: public
 e_f
,
 public
 g_h 
{
};

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2007-01-03 21:11 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-12-27 10:32 Some problems in `add-log-current-defun' Herbert Euler
2006-12-27 10:45 ` David Kastrup
2006-12-27 11:48   ` Masatake YAMATO
2006-12-27 12:22     ` David Kastrup
2006-12-27 11:55 ` Masatake YAMATO
2006-12-27 13:46 ` Masatake YAMATO
2006-12-27 21:17 ` Richard Stallman
2006-12-28 12:41   ` Masatake YAMATO
2006-12-29 15:44     ` Richard Stallman
2006-12-30  4:55       ` Masatake YAMATO
2006-12-30 18:24         ` Richard Stallman
2006-12-28 12:47   ` Herbert Euler
2006-12-29 15:44     ` Richard Stallman
2006-12-29  3:10   ` Herbert Euler
  -- strict thread matches above, loose matches on Subject: below --
2006-12-31  8:28 Herbert Euler
2006-12-31 22:13 ` Richard Stallman
2007-01-01  1:27   ` Herbert Euler
2007-01-02 23:35   ` Stefan Monnier
2007-01-03 21:11     ` Richard Stallman

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).