Skip to content

[emacs][clang-format] Add elisp API for clang-format on git diffs #112792

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.

Sign up for GitHub

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

Merged
merged 17 commits into from
Feb 4, 2025
Merged

[emacs][clang-format] Add elisp API for clang-format on git diffs #112792

Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
802764e
[emacs][clang-format] Add elisp API for clang-format on git diffs
goldsteinn Oct 17, 2024
2c8395d
Address V2 Comments
goldsteinn Nov 3, 2024
fc05d68
Rename 'git' -> 'vc' and 'diffs' -> 'diff'
goldsteinn Nov 3, 2024
36aa80b
Complete sentences/byte-compile/refactor vc check
goldsteinn Nov 4, 2024
fcf504f
use diff-no-select
goldsteinn Nov 4, 2024
b42785d
Revert "use diff-no-select"
goldsteinn Nov 4, 2024
95db21c
comment why no diff-no-select
goldsteinn Nov 4, 2024
98d5894
Use 'diff-command' from diff.el
goldsteinn Nov 4, 2024
1e65249
git-vc-program/ignore errors
goldsteinn Nov 5, 2024
7cdfa4e
Ignore errors deleting both
goldsteinn Nov 5, 2024
6d49dcf
Nits/Diff command that doesn't req regex
goldsteinn Nov 5, 2024
7742bd1
Use macro for cleaning files
goldsteinn Nov 7, 2024
9ae26d4
Compat Dif
goldsteinn Nov 11, 2024
e547d0e
Optimize diff search
goldsteinn Nov 11, 2024
b277560
Use re-search-format
goldsteinn Nov 18, 2024
8c8c551
Update regex/getting matches
goldsteinn Nov 19, 2024
92247c1
Address Comments
goldsteinn Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 176 additions & 14 deletions clang/tools/clang-format/clang-format.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,18 +146,118 @@ is a zero-based file offset, assuming ‘utf-8-unix’ coding."
(lambda (byte &optional _quality _coding-system)
(byte-to-position (1+ byte)))))

;;;###autoload
(defun clang-format-region (start end &optional style assume-file-name)
"Use clang-format to format the code between START and END according to STYLE.
If called interactively uses the region or the current statement if there is no
no active region. If no STYLE is given uses `clang-format-style'. Use
ASSUME-FILE-NAME to locate a style config file, if no ASSUME-FILE-NAME is given
uses the function `buffer-file-name'."
(interactive
(if (use-region-p)
(list (region-beginning) (region-end))
(list (point) (point))))
(defun clang-format--vc-diff-match-diff-line (line)
;; We are matching something like:
;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@"
;; Return as "<LineStart>:<LineEnd>"
(when (string-match "^@@\s-[0-9,]+\s\\+\\([0-9]+\\)\\(,\\([0-9]+\\)\\)?\s@@$" line)
;; If we have multi-line diff
(if (match-string 3 line)
(concat (match-string 1 line)
":"
(number-to-string
(+ (string-to-number (match-string 1 line))
(string-to-number (match-string 3 line)))))
(concat (match-string 1 line) ":" (match-string 1 line)))))

(defun clang-format--vc-diff-get-diff-lines (file-orig file-new)
Copy link
Contributor

@ideasman42 ideasman42 Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

*picky* - personal preference, but I find it odd that the function returns command line arguments for clang-format.

It can return a list of cons-pairs of instead (list (cons int int) ...) instead, then the clang-format function can convert them to the arguments used by clang-format.

(the patch I attached did this), it's not a big change, I'd expect a formatting function that can operate on multiple ranges to take int-pairs instead of a list of strings.

"Return all line regions that contain diffs between FILE-ORIG and
FILE-NEW. If there is no diff ‘nil’ is returned. Otherwise the
return is a ‘list’ of lines in the format ‘--lines=<start>:<end>’
which can be passed directly to ‘clang-format’."
;; Use temporary buffer for output of diff.
(with-temp-buffer
(let ((status (call-process
"diff"
nil
(current-buffer)
nil
;; Binary diff has different behaviors that we
;; aren't interested in.
"-a"
;; Get minimal diff (copy diff config for git-clang-format).
"-U0"
file-orig
file-new))
(stderr (concat (if (zerop (buffer-size)) "" ": ")
(buffer-substring-no-properties
(point-min) (line-end-position))))
(diff-lines '()))
(cond
((stringp status)
(error "(diff killed by signal %s%s)" status stderr))
;; Return of 0 indicates no diff.
((= status 0) nil)
;; Return of 1 indicates found diffs and no error.
((= status 1)
;; Iterate through all lines in diff buffer and collect all
;; lines in current buffer that have a diff.
(goto-char (point-min))
(while (not (eobp))
Copy link
Contributor

@ideasman42 ideasman42 Nov 5, 2024

Choose a reason for hiding this comment

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

This is quite inefficient, I'd suspect with a large diff this would be a bottleneck.

Instead of stepping over and extracting every line, then running a regex match against it.
It's much more efficient to use (while (re-search-forward ...).

Copy link
Contributor Author

@goldsteinn goldsteinn Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I think I can go back to using the no-diff version i.e:

diff -a orig.cpp ../llvm/lib/Analysis/ValueTracking.cpp --changed-group-format="%(N=0?:--lines=%dF:%dM " --unchanged-group-format=

Original issue was I didn't have the N=0, so deleted lines cause issue, but AFAICT this works fine.

Copy link
Contributor Author

@goldsteinn goldsteinn Nov 5, 2024

Choose a reason for hiding this comment

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

Changed to new diff command so that and below comment should be resolved. Going to resolve below, if you have any doubts about new command/etc... we can discuss them here.

Copy link
Contributor Author

@goldsteinn goldsteinn Nov 11, 2024

Choose a reason for hiding this comment

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

Update to essentially this (although by hand with string-match) as that was about 50% faster. I think because we drop string-properties but not really certain why.

(let ((diff-line (clang-format--vc-diff-match-diff-line
(buffer-substring-no-properties
(line-beginning-position)
(line-end-position)))))
(when diff-line
;; Create list line regions with diffs to pass to
;; clang-format.
(push (concat "--lines=" diff-line) diff-lines)))
(forward-line 1))
(reverse diff-lines))
;; Any return != 0 && != 1 indicates some level of error.
(t
(error "(diff returned unsuccessfully %s%s)" status stderr))))))
Copy link
Contributor

@ideasman42 ideasman42 Jan 16, 2025

Choose a reason for hiding this comment

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

All errors should use the "clang-format: " prefix, otherwise it can be difficult to track down errors when these functions are called indirectly.


(defun clang-format--vc-diff-get-vc-head-file (tmpfile-vc-head)
"Stores the contents of ‘buffer-file-name’ at vc revision HEAD into
‘tmpfile-vc-head’. If the current buffer is either not a file or not
in a vc repo, this results in an error. Currently git is the only
supported vc."
;; We need the current buffer to be a file.
(unless (buffer-file-name)
(error "Buffer is not visiting a file"))

(let ((base-dir (vc-root-dir))
(backend (vc-backend (buffer-file-name))))
;; We need to be able to find version control (git) root.
(unless base-dir
(error "File not known to git"))
(cond
((string-equal backend "Git")
;; Get the filename relative to git root.
(let ((vc-file-name (substring
(expand-file-name (buffer-file-name))
(string-width (expand-file-name base-dir))
nil)))
(let ((status (call-process
"git"
nil
`(:file, tmpfile-vc-head)
nil
"show" (concat "HEAD:" vc-file-name)))
(stderr (with-temp-buffer
(unless (zerop (cadr (insert-file-contents tmpfile-vc-head)))
(insert ": "))
(buffer-substring-no-properties
(point-min) (line-end-position)))))
(when (stringp status)
(error "(git show HEAD:%s killed by signal %s%s)"
vc-file-name status stderr))
(unless (zerop status)
(error "(git show HEAD:%s returned unsuccessfully %s%s)"
vc-file-name status stderr)))))
(t
(error
"Version control %s isn't supported, currently supported backends: git"
backend)))))


(defun clang-format--region-impl (start end &optional style assume-file-name lines)
"Common implementation for ‘clang-format-buffer’,
‘clang-format-region’, and ‘clang-format-vc-diff’. START and END
refer to the region to be formatter. STYLE and ASSUME-FILE-NAME are
used for configuring the clang-format. And LINES is used to pass
specific locations for reformatting (i.e diff locations)."
(unless style
(setq style clang-format-style))

Expand Down Expand Up @@ -190,8 +290,12 @@ uses the function `buffer-file-name'."
(list "--assume-filename" assume-file-name))
,@(and style (list "--style" style))
"--fallback-style" ,clang-format-fallback-style
"--offset" ,(number-to-string file-start)
"--length" ,(number-to-string (- file-end file-start))
,@(and lines lines)
,@(and (not lines)
(list
"--offset" (number-to-string file-start)
"--length" (number-to-string
(- file-end file-start))))
"--cursor" ,(number-to-string cursor))))
(stderr (with-temp-buffer
(unless (zerop (cadr (insert-file-contents temp-file)))
Expand Down Expand Up @@ -219,14 +323,72 @@ uses the function `buffer-file-name'."
(delete-file temp-file)
(when (buffer-name temp-buffer) (kill-buffer temp-buffer)))))


;;;###autoload
(defun clang-format-vc-diff (&optional style assume-file-name)
"The same as ‘clang-format-buffer’ but only operates on the vc
diffs from HEAD in the buffer. If no STYLE is given uses
‘clang-format-style’. Use ASSUME-FILE-NAME to locate a style config
file. If no ASSUME-FILE-NAME is given uses the function
‘buffer-file-name’."
(interactive)
(let ((tmpfile-vc-head nil)
(tmpfile-curbuf nil))
(unwind-protect
Copy link
Contributor

@ideasman42 ideasman42 Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

*picky* I find it often works better to wrap unwind-protect in a macro.

Here is a macro that declares a list where temporary files are added & automatically cleaned up.

(defmacro clang-format--with-delete-files-guard (bind-files-to-delete &rest body)
  "Execute BODY which may add temp files to BIND-FILES-TO-DELETE."
  (declare (indent 1))
  `(let ((,bind-files-to-delete nil))
     (unwind-protect
         (progn
           ,@body)
       (while ,bind-files-to-delete
         (with-demoted-errors "failed to remove file: %S"
           (delete-file (pop ,bind-files-to-delete)))))))

Example usage

(clang-format--with-delete-files-guard temp-files
  ... snip ...
  (setq tmpfile-curbuf (make-temp-file "clang-format-vc-tmp"))
  ;; Push to the temp file list ensures it gets cleaned up.
  (push tmpfile-curbuf temp-files)
  ... snip ...
  )

Copy link
Contributor Author

@goldsteinn goldsteinn Nov 5, 2024

Choose a reason for hiding this comment

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

IMO, since we only do it once, macro doesn't really simplify anything.

Copy link
Contributor

@ideasman42 ideasman42 Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It de-duplicates 1 removal, and allows other files to be created in a scope where they can be easily removed.

Personally I find it helps readability ... (with-delete-files-guard temp-files ...) is quite clear, where as (let (...) (unwind-protect ((progn ...) (... actual cleanup ...))) is overly nested and is iteself not all that readable.

The macro encapsulates something that is otherwise a bit awkward and clearly separates responsibilities.

Having said that, I don't consider it that big of a deal, just explaining why I find it an improvement.

Copy link
Contributor Author

@goldsteinn goldsteinn Nov 5, 2024

Choose a reason for hiding this comment

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

Okay, and we could also use macro in clang-format--region-impl so my argument is blown up.

Copy link
Contributor Author

@goldsteinn goldsteinn Nov 5, 2024

Choose a reason for hiding this comment

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

Actually going to keep clang-format--region-impl unchanged, there are also buffers that need to be cleaned up so would need the unwind-project either way.

(progn
(setq tmpfile-vc-head
(make-temp-file "clang-format-vc-tmp-head-content"))
(clang-format--vc-diff-get-vc-head-file tmpfile-vc-head)
;; Move the current buffer to a temporary file to take a
;; diff. Even if current-buffer is backed by a file, we
;; want to diff the buffer contents which might not be
;; saved.
(setq tmpfile-curbuf (make-temp-file "clang-format-vc-tmp"))
Copy link
Contributor

@ideasman42 ideasman42 Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggest to pass this tmpfile-curbuf to clang-format--vc-diff-get-vc-head-file, then use (:file FILE) as the destination.
This will be faster and use less memory as it will do process->file instead of process->buffer->file.

Copy link
Contributor Author

@goldsteinn goldsteinn Nov 5, 2024

Choose a reason for hiding this comment

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

Hmm? In clang-format--vc-diff-get-vc-head-file we do: ``(:file, tmpfile-vc-head), tmpfile-curbuf` is the current contents of the buffer we are calling `clang-format-vc-diff` on.

Copy link
Contributor

@ideasman42 ideasman42 Nov 5, 2024

Choose a reason for hiding this comment

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

Ah, sorry - my mistake.

(write-region nil nil tmpfile-curbuf nil 'nomessage)
;; Get a list of lines with a diff.
(let ((diff-lines
(clang-format--vc-diff-get-diff-lines
tmpfile-vc-head tmpfile-curbuf)))
;; If we have any diffs, format them.
(when diff-lines
(clang-format--region-impl
(point-min)
(point-max)
style
assume-file-name
diff-lines))))
(progn
;; Cleanup temporary files we created.
(when tmpfile-vc-head (delete-file tmpfile-vc-head))
(when tmpfile-curbuf (delete-file tmpfile-curbuf))))))


;;;###autoload
(defun clang-format-region (start end &optional style assume-file-name)
"Use clang-format to format the code between START and END according
to STYLE. If called interactively uses the region or the current
statement if there is no no active region. If no STYLE is given uses
`clang-format-style'. Use ASSUME-FILE-NAME to locate a style config
file, if no ASSUME-FILE-NAME is given uses the function
`buffer-file-name'."
(interactive
(if (use-region-p)
(list (region-beginning) (region-end))
(list (point) (point))))
(clang-format--region-impl start end style assume-file-name))

;;;###autoload
(defun clang-format-buffer (&optional style assume-file-name)
"Use clang-format to format the current buffer according to STYLE.
If no STYLE is given uses `clang-format-style'. Use ASSUME-FILE-NAME
to locate a style config file. If no ASSUME-FILE-NAME is given uses
the function `buffer-file-name'."
(interactive)
(clang-format-region (point-min) (point-max) style assume-file-name))
(clang-format--region-impl
(point-min)
(point-max)
style
assume-file-name))

;;;###autoload
(defalias 'clang-format 'clang-format-region)
Expand Down
Loading