[clang] Add "clang-format-on-save-mode" minor mode to clang-format.el (PR #104533)
ideasman42 wrote: Is there any action needed on my side? (the PR was approved over a week ago). https://github.com/llvm/llvm-project/pull/104533 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add "clang-format-on-save-mode" minor mode to clang-format.el (PR #104533)
https://github.com/ideasman42 created https://github.com/llvm/llvm-project/pull/104533 Add a minor mode to run clang-format on save. Formatting before saving works well and is convenient to avoid having to remember to manually run clang format. I've written this as it's own package but it's probably better if the functionality is supported by clang-format.el. See: https://github.com/melpa/melpa/pull/8762 >From d53cf4f059cacadbbb6e32349a26580e7ea1a55b Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Fri, 16 Aug 2024 11:28:19 +1000 Subject: [PATCH] Add "clang-format-on-save-mode" minor mode to clang-format.el Add a minor mode to run clang-format on save. --- clang/tools/clang-format/clang-format.el | 59 1 file changed, 59 insertions(+) diff --git a/clang/tools/clang-format/clang-format.el b/clang/tools/clang-format/clang-format.el index f43bf063c62970..25a5865efdebd2 100644 --- a/clang/tools/clang-format/clang-format.el +++ b/clang/tools/clang-format/clang-format.el @@ -70,6 +70,20 @@ in such buffers." :safe #'stringp) (make-variable-buffer-local 'clang-format-fallback-style) +(defcustom clang-format-on-save-p 'clang-format-on-save-check-config-exists + "Only reformat on save if this function returns non-nil. + +You may wish to choose one of the following options: +- `always': To always format on save. +- `clang-format-on-save-check-config-exists': + Only reformat when \".clang-format\" exists. + +Otherwise you can set this to a user defined function." + :group 'clang-format + :type 'function + :risky t) +(make-variable-buffer-local 'clang-format-on-save-p) + (defun clang-format--extract (xml-node) "Extract replacements and cursor information from XML-NODE." (unless (and (listp xml-node) (eq (xml-node-name xml-node) 'replacements)) @@ -217,5 +231,50 @@ the function `buffer-file-name'." ;;;###autoload (defalias 'clang-format 'clang-format-region) +;; Format on save minor mode. +;; +;; Optional minor mode for formatting on save. + +(defun clang-format--on-save-buffer-hook () + "The hook to run on buffer saving to format the buffer." + ;; Demote errors as this is user configurable, we can't be sure it wont error. + (when (with-demoted-errors "clang-format-on-save: Error %S" + (funcall clang-format-on-save-p)) +(clang-format-buffer)) + ;; Continue to save. + nil) + +(defun clang-format--on-save-enable () + "Disable the minor mode." + (add-hook 'before-save-hook #'clang-format--on-save-buffer-hook nil t)) + +(defun clang-format--on-save-disable () + "Enable the minor mode." + (remove-hook 'before-save-hook #'clang-format--on-save-buffer-hook t)) + +;; Default value for `clang-format-on-save-p'. +(defun clang-format-on-save-check-config-exists () + "Return non-nil when `.clang-format' is found in a parent directory." + ;; Unlikely but possible this is nil. + (let ((filepath buffer-file-name)) +(cond + (filepath + (null (null (locate-dominating-file (file-name-directory filepath) ".clang-format" + (t + nil + +;;;###autoload +(define-minor-mode clang-format-on-save-mode + "Clang-format on save minor mode." + :global nil + :lighter "" + :keymap nil + + (cond + (clang-format-on-save-mode +(clang-format--on-save-enable)) + (t +(clang-format--on-save-disable + (provide 'clang-format) ;;; clang-format.el ends here ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add "clang-format-on-save-mode" minor mode to clang-format.el (PR #104533)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/104533 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add "clang-format-on-save-mode" minor mode to clang-format.el (PR #104533)
https://github.com/ideasman42 updated https://github.com/llvm/llvm-project/pull/104533 >From d53cf4f059cacadbbb6e32349a26580e7ea1a55b Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Fri, 16 Aug 2024 11:28:19 +1000 Subject: [PATCH 1/2] Add "clang-format-on-save-mode" minor mode to clang-format.el Add a minor mode to run clang-format on save. --- clang/tools/clang-format/clang-format.el | 59 1 file changed, 59 insertions(+) diff --git a/clang/tools/clang-format/clang-format.el b/clang/tools/clang-format/clang-format.el index f43bf063c62970..25a5865efdebd2 100644 --- a/clang/tools/clang-format/clang-format.el +++ b/clang/tools/clang-format/clang-format.el @@ -70,6 +70,20 @@ in such buffers." :safe #'stringp) (make-variable-buffer-local 'clang-format-fallback-style) +(defcustom clang-format-on-save-p 'clang-format-on-save-check-config-exists + "Only reformat on save if this function returns non-nil. + +You may wish to choose one of the following options: +- `always': To always format on save. +- `clang-format-on-save-check-config-exists': + Only reformat when \".clang-format\" exists. + +Otherwise you can set this to a user defined function." + :group 'clang-format + :type 'function + :risky t) +(make-variable-buffer-local 'clang-format-on-save-p) + (defun clang-format--extract (xml-node) "Extract replacements and cursor information from XML-NODE." (unless (and (listp xml-node) (eq (xml-node-name xml-node) 'replacements)) @@ -217,5 +231,50 @@ the function `buffer-file-name'." ;;;###autoload (defalias 'clang-format 'clang-format-region) +;; Format on save minor mode. +;; +;; Optional minor mode for formatting on save. + +(defun clang-format--on-save-buffer-hook () + "The hook to run on buffer saving to format the buffer." + ;; Demote errors as this is user configurable, we can't be sure it wont error. + (when (with-demoted-errors "clang-format-on-save: Error %S" + (funcall clang-format-on-save-p)) +(clang-format-buffer)) + ;; Continue to save. + nil) + +(defun clang-format--on-save-enable () + "Disable the minor mode." + (add-hook 'before-save-hook #'clang-format--on-save-buffer-hook nil t)) + +(defun clang-format--on-save-disable () + "Enable the minor mode." + (remove-hook 'before-save-hook #'clang-format--on-save-buffer-hook t)) + +;; Default value for `clang-format-on-save-p'. +(defun clang-format-on-save-check-config-exists () + "Return non-nil when `.clang-format' is found in a parent directory." + ;; Unlikely but possible this is nil. + (let ((filepath buffer-file-name)) +(cond + (filepath + (null (null (locate-dominating-file (file-name-directory filepath) ".clang-format" + (t + nil + +;;;###autoload +(define-minor-mode clang-format-on-save-mode + "Clang-format on save minor mode." + :global nil + :lighter "" + :keymap nil + + (cond + (clang-format-on-save-mode +(clang-format--on-save-enable)) + (t +(clang-format--on-save-disable + (provide 'clang-format) ;;; clang-format.el ends here >From a3e87f43933a80cca51bc002ed9d4801b18b6b22 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Fri, 16 Aug 2024 11:47:59 +1000 Subject: [PATCH 2/2] Minor cleanup. --- clang/tools/clang-format/clang-format.el | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clang/tools/clang-format/clang-format.el b/clang/tools/clang-format/clang-format.el index 25a5865efdebd2..64d1d3f5789c72 100644 --- a/clang/tools/clang-format/clang-format.el +++ b/clang/tools/clang-format/clang-format.el @@ -232,8 +232,6 @@ the function `buffer-file-name'." (defalias 'clang-format 'clang-format-region) ;; Format on save minor mode. -;; -;; Optional minor mode for formatting on save. (defun clang-format--on-save-buffer-hook () "The hook to run on buffer saving to format the buffer." @@ -259,7 +257,7 @@ the function `buffer-file-name'." (let ((filepath buffer-file-name)) (cond (filepath - (null (null (locate-dominating-file (file-name-directory filepath) ".clang-format" + (not (null (locate-dominating-file (file-name-directory filepath) ".clang-format" (t nil ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add "clang-format-on-save-mode" minor mode to clang-format.el (PR #104533)
ideasman42 wrote: > I'm not an emacs user and I'm unsure if the other maintainers are. If you are > happy to address any issues that might arise I'd be happy for this to land. Yes, I'm happy to resolve any issues - for some context, I've been using a version of this for some years. Originally from this answer https://emacs.stackexchange.com/q/48500/2418 The minor mode just makes it convenient to enable/disable - I find it especially when working on some projects that don't use clang-format to optionally check for `.clang-format` and only format-on-save in this case (that behavior can be configured too). https://github.com/llvm/llvm-project/pull/104533 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add "clang-format-on-save-mode" minor mode to clang-format.el (PR #104533)
https://github.com/ideasman42 updated https://github.com/llvm/llvm-project/pull/104533 >From d53cf4f059cacadbbb6e32349a26580e7ea1a55b Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Fri, 16 Aug 2024 11:28:19 +1000 Subject: [PATCH 1/3] Add "clang-format-on-save-mode" minor mode to clang-format.el Add a minor mode to run clang-format on save. --- clang/tools/clang-format/clang-format.el | 59 1 file changed, 59 insertions(+) diff --git a/clang/tools/clang-format/clang-format.el b/clang/tools/clang-format/clang-format.el index f43bf063c62970..25a5865efdebd2 100644 --- a/clang/tools/clang-format/clang-format.el +++ b/clang/tools/clang-format/clang-format.el @@ -70,6 +70,20 @@ in such buffers." :safe #'stringp) (make-variable-buffer-local 'clang-format-fallback-style) +(defcustom clang-format-on-save-p 'clang-format-on-save-check-config-exists + "Only reformat on save if this function returns non-nil. + +You may wish to choose one of the following options: +- `always': To always format on save. +- `clang-format-on-save-check-config-exists': + Only reformat when \".clang-format\" exists. + +Otherwise you can set this to a user defined function." + :group 'clang-format + :type 'function + :risky t) +(make-variable-buffer-local 'clang-format-on-save-p) + (defun clang-format--extract (xml-node) "Extract replacements and cursor information from XML-NODE." (unless (and (listp xml-node) (eq (xml-node-name xml-node) 'replacements)) @@ -217,5 +231,50 @@ the function `buffer-file-name'." ;;;###autoload (defalias 'clang-format 'clang-format-region) +;; Format on save minor mode. +;; +;; Optional minor mode for formatting on save. + +(defun clang-format--on-save-buffer-hook () + "The hook to run on buffer saving to format the buffer." + ;; Demote errors as this is user configurable, we can't be sure it wont error. + (when (with-demoted-errors "clang-format-on-save: Error %S" + (funcall clang-format-on-save-p)) +(clang-format-buffer)) + ;; Continue to save. + nil) + +(defun clang-format--on-save-enable () + "Disable the minor mode." + (add-hook 'before-save-hook #'clang-format--on-save-buffer-hook nil t)) + +(defun clang-format--on-save-disable () + "Enable the minor mode." + (remove-hook 'before-save-hook #'clang-format--on-save-buffer-hook t)) + +;; Default value for `clang-format-on-save-p'. +(defun clang-format-on-save-check-config-exists () + "Return non-nil when `.clang-format' is found in a parent directory." + ;; Unlikely but possible this is nil. + (let ((filepath buffer-file-name)) +(cond + (filepath + (null (null (locate-dominating-file (file-name-directory filepath) ".clang-format" + (t + nil + +;;;###autoload +(define-minor-mode clang-format-on-save-mode + "Clang-format on save minor mode." + :global nil + :lighter "" + :keymap nil + + (cond + (clang-format-on-save-mode +(clang-format--on-save-enable)) + (t +(clang-format--on-save-disable + (provide 'clang-format) ;;; clang-format.el ends here >From a3e87f43933a80cca51bc002ed9d4801b18b6b22 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Fri, 16 Aug 2024 11:47:59 +1000 Subject: [PATCH 2/3] Minor cleanup. --- clang/tools/clang-format/clang-format.el | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clang/tools/clang-format/clang-format.el b/clang/tools/clang-format/clang-format.el index 25a5865efdebd2..64d1d3f5789c72 100644 --- a/clang/tools/clang-format/clang-format.el +++ b/clang/tools/clang-format/clang-format.el @@ -232,8 +232,6 @@ the function `buffer-file-name'." (defalias 'clang-format 'clang-format-region) ;; Format on save minor mode. -;; -;; Optional minor mode for formatting on save. (defun clang-format--on-save-buffer-hook () "The hook to run on buffer saving to format the buffer." @@ -259,7 +257,7 @@ the function `buffer-file-name'." (let ((filepath buffer-file-name)) (cond (filepath - (null (null (locate-dominating-file (file-name-directory filepath) ".clang-format" + (not (null (locate-dominating-file (file-name-directory filepath) ".clang-format" (t nil >From df069cc028cb0eff6910836c4270335b11c3ec0f Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Fri, 16 Aug 2024 21:26:20 +1000 Subject: [PATCH 3/3] Shorten overly verbose message --- clang/tools/clang-format/clang-format.el | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/tools/clang-format/clang-format.el b/clang/tools/clang-format/clang-format.el index 64d1d3f5789c72..5cdd984c531ec4 100644 --- a/clang/tools/clang-format/clang-format.el +++ b/clang/tools/clang-format/clang-format.el @@ -236,7 +236,7 @@ the function `buffer-file-name'." (defun clang-format--on-save-buffer-hook () "The hook to run on buffer saving to format the buffer." ;; Demote errors as this is user configurable, we can't be sure it wont error. - (when (wi
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,115 @@ 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) + ;; Matching something like: + ;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@" + ;; Return as ":" + (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) + "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=:' +which can be passed directly to 'clang-format'" + ;; Temporary buffer for output of diff. + (with-temp-buffer +(let ((status (call-process ideasman42 wrote: Ah, I wasn't aware of that, even so, there is some advantage in using emacs diff functionality as it handles error cases, removing temp files etc. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -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 ":" + (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) + "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=:’ +which can be passed directly to ‘clang-format’." + ;; Use temporary buffer for output of diff. + (with-temp-buffer +(let ((status (call-process + "diff" ideasman42 wrote: While I think it may be best to call `diff` functions directly, if this is call is kept, it's probably best to use `diff-command`, so the diff command used by `vc/diff.el` is used here, it may be necessary on WIN32. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,115 @@ 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) + ;; Matching something like: + ;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@" + ;; Return as ":" + (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) + "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=:' +which can be passed directly to 'clang-format'" + ;; Temporary buffer for output of diff. + (with-temp-buffer +(let ((status (call-process ideasman42 wrote: Regarding temporary file use: did you check on the `diff-buffers` function? it should be possible to diff two temporary buffers without creating temporary files. Looking into the implementation `diff-no-select` may be what your after as that looks to be intended for non interactive use. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,115 @@ 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) + ;; Matching something like: + ;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@" + ;; Return as ":" + (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) + "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=:' +which can be passed directly to 'clang-format'" + ;; 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)) + (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 + (add-to-list 'diff-lines (concat "--lines=" diff-line) t))) + (forward-line 1)) +diff-lines) + ;; Any return != 0 && != 1 indicates some level of error + (t +(error "(diff returned unsuccessfully %s%s)" status stderr)) + +(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." + ;; Needs current buffer to be a file + (unless (buffer-file-name) +(error "Buffer is not visiting a file")) + ;; Only version control currently supported is Git + (unless (string-equal (vc-backend (buffer-file-name)) "Git") ideasman42 wrote: Suggestion: my preference would be to write this in a way that makes adding other VC more straightforward. ``` (let ((vc-backend ...snip...)) (cond (string-equal vc-backend "Git") ...git...logic... (t (error "Version control %s isn't supported, currently supported backends ... snip ..." vc-backend ``` https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,115 @@ 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) + ;; Matching something like: + ;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@" + ;; Return as ":" + (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) + "Return all line regions that contain diffs between FILE-ORIG and ideasman42 wrote: Running `byte-compile-file` gives various warnings: ``` clang-format.el:163:2: Warning: docstring has wrong usage of unescaped single quotes (use \=' or different quoting such as `...') clang-format.el:201:19: Error: ‘add-to-list’ can’t use lexical var ‘diff-lines’; use ‘push’ or ‘cl-pushnew’ In clang-format--vc-diff-get-vc-head-file: clang-format.el:211:2: Warning: docstring has wrong usage of unescaped single quotes (use \=' or different quoting such as `...') In clang-format--region-impl: clang-format.el:252:2: Warning: docstring has wrong usage of unescaped single quotes (use \=' or different quoting such as `...') In clang-format-vc-diff: clang-format.el:325:2: Warning: docstring has wrong usage of unescaped single quotes (use \=' or different quoting such as `...') ``` https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 requested changes to this pull request. This seems more ore less OK, although https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,115 @@ 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) + ;; Matching something like: + ;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@" + ;; Return as ":" + (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) + "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=:' +which can be passed directly to 'clang-format'" + ;; 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)) + (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 + (add-to-list 'diff-lines (concat "--lines=" diff-line) t))) + (forward-line 1)) +diff-lines) + ;; Any return != 0 && != 1 indicates some level of error + (t +(error "(diff returned unsuccessfully %s%s)" status stderr)) + +(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." + ;; Needs current buffer to be a file + (unless (buffer-file-name) +(error "Buffer is not visiting a file")) + ;; Only version control currently supported is Git + (unless (string-equal (vc-backend (buffer-file-name)) "Git") +(error "Not using git")) + + (let ((base-dir (vc-root-dir))) +;; Need to be able to find version control (git) root ideasman42 wrote: \*picky\* use full sentences for comments - end with a full-stop, applies to many other comments here. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -312,15 +335,35 @@ diffs from HEAD in the buffer. If no STYLE is given uses file. If no ASSUME-FILE-NAME is given uses the function ‘buffer-file-name’." (interactive) - (let ((diff-lines (clang-format--vc-diff-get-diff-lines))) -;; If we have any diffs, format them. -(when diff-lines - (clang-format--region-impl - (point-min) - (point-max) - style - assume-file-name - diff-lines + (let ((tmpfile-vc-head nil) +(tmpfile-curbuf nil)) +(unwind-protect ideasman42 wrote: \*picky\* I find it often works better to wrap `unwind-protect` in a macro. Example usage ``` (clang-format--with-delete-files-guard files-to-delete ... snip ... (push (setq tmpfile-curbuf (make-temp-file "clang-format-vc-tmp")) temp-files) (push tmpfile-curbuf temp-files)) ```k Here is a macro that does this: ``` (defmacro clang-format--with-advice (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))) ``` https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,115 @@ 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) + ;; Matching something like: + ;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@" + ;; Return as ":" + (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) + "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=:' +which can be passed directly to 'clang-format'" + ;; Temporary buffer for output of diff. + (with-temp-buffer +(let ((status (call-process ideasman42 wrote: In this case, include a code-comment for why `diff-no-select` is not used. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -132,18 +132,97 @@ 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--git-diffs-get-diff-lines (file-orig file-new) + "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=:' +which can be passed directly to 'clang-format'" + ;; 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" + ;; Printout changes as only the line groups. + "--changed-group-format=--lines=%dF:%dL " + ;; Ignore unchanged content. + "--unchanged-group-format=" + file-orig + file-new ideasman42 wrote: \*picky\* don't use dangling parenthesis. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -132,18 +132,97 @@ 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--git-diffs-get-diff-lines (file-orig file-new) + "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=:' +which can be passed directly to 'clang-format'" + ;; 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" + ;; Printout changes as only the line groups. + "--changed-group-format=--lines=%dF:%dL " + ;; Ignore unchanged content. + "--unchanged-group-format=" + file-orig + file-new + ) + ) + (stderr (concat (if (zerop (buffer-size)) "" ": ") + (buffer-substring-no-properties + (point-min) (line-end-position) + (when (stringp status) +(error "(diff killed by signal %s%s)" status stderr)) + (unless (= status 0) +(unless (= status 1) + (error "(diff returned unsuccessfully %s%s)" status stderr))) + + + (if (= status 0) + ;; Status == 0 -> no Diff. + nil +(progn + ;; Split "--lines=:... --lines=:" output to + ;; a list for return. + (s-split + " " + (string-trim +(buffer-substring-no-properties + (point-min) (point-max) + +(defun clang-format--git-diffs-get-git-head-file () + "Returns a temporary file with the content of 'buffer-file-name' at +git revision HEAD. If the current buffer is either not a file or not +in a git repo, this results in an error" + ;; Needs current buffer to be a file + (unless (buffer-file-name) +(error "Buffer is not visiting a file")) + ;; Need to be able to find version control (git) root + (unless (vc-root-dir) +(error "File not known to git")) + ;; Need version control to in fact be git + (unless (string-equal (vc-backend (buffer-file-name)) "Git") +(error "Not using git")) + + (let ((tmpfile-git-head (make-temp-file "clang-format-tmp-git-head-content"))) ideasman42 wrote: >From what I can tell this temporary file is never deleted, further, any errors >after creating the temporary file would leave it created. Without attempting to do this myself, I'm not sure of the best solution but suggest: `(with-temp-file ...)` The caller would need to use this and pass in the temporary file file. If the behavior of `with-temp-file` isn't what your after, you could write your own macro that creates a scoped temporary file that gets removed in case of errors. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -132,18 +132,97 @@ 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--git-diffs-get-diff-lines (file-orig file-new) + "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=:' +which can be passed directly to 'clang-format'" + ;; 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" + ;; Printout changes as only the line groups. + "--changed-group-format=--lines=%dF:%dL " + ;; Ignore unchanged content. + "--unchanged-group-format=" + file-orig + file-new + ) + ) + (stderr (concat (if (zerop (buffer-size)) "" ": ") + (buffer-substring-no-properties + (point-min) (line-end-position) + (when (stringp status) +(error "(diff killed by signal %s%s)" status stderr)) + (unless (= status 0) +(unless (= status 1) + (error "(diff returned unsuccessfully %s%s)" status stderr))) + + + (if (= status 0) + ;; Status == 0 -> no Diff. + nil +(progn + ;; Split "--lines=:... --lines=:" output to + ;; a list for return. + (s-split + " " + (string-trim +(buffer-substring-no-properties + (point-min) (point-max) + +(defun clang-format--git-diffs-get-git-head-file () + "Returns a temporary file with the content of 'buffer-file-name' at +git revision HEAD. If the current buffer is either not a file or not +in a git repo, this results in an error" + ;; Needs current buffer to be a file + (unless (buffer-file-name) +(error "Buffer is not visiting a file")) + ;; Need to be able to find version control (git) root + (unless (vc-root-dir) ideasman42 wrote: \*picky\* assign this to a variable and reuse it instead of calling twice. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 requested changes to this pull request. Note that I'm not a regular clang contributor, I just submitted a small improvement to clang-format, and maintain some emacs packages in elpa & melpa. Overall the PR looks like it needs more attention to detail, as far as I can tell it's creating temporary files and never removing them, various minor issues noted inline. - This PR needs to be rebased on top of the recently added `clang-format-on-save-mode` commit. - Running `package-lint` reports. ``` 156:19: warning: Closing parens should not be wrapped onto new lines. 157:18: warning: Closing parens should not be wrapped onto new lines. 176:12: error: You should depend on (emacs "24.4") or the compat package if you need `string-trim'. 188:11: error: You should depend on (emacs "25.1") if you need `vc-root-dir'. 198:59: error: You should depend on (emacs "25.1") if you need `vc-root-dir'. ``` https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -132,18 +132,97 @@ 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--git-diffs-get-diff-lines (file-orig file-new) + "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=:' +which can be passed directly to 'clang-format'" + ;; 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" + ;; Printout changes as only the line groups. + "--changed-group-format=--lines=%dF:%dL " + ;; Ignore unchanged content. + "--unchanged-group-format=" + file-orig + file-new + ) + ) + (stderr (concat (if (zerop (buffer-size)) "" ": ") + (buffer-substring-no-properties + (point-min) (line-end-position) + (when (stringp status) +(error "(diff killed by signal %s%s)" status stderr)) + (unless (= status 0) +(unless (= status 1) + (error "(diff returned unsuccessfully %s%s)" status stderr))) + + + (if (= status 0) + ;; Status == 0 -> no Diff. + nil +(progn + ;; Split "--lines=:... --lines=:" output to + ;; a list for return. + (s-split ideasman42 wrote: `s-split` is not part of emacs standard library, depending on https://github.com/magnars/s.el doesn't seem worthwhile, use built-in emacs functionality. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -132,18 +132,97 @@ 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--git-diffs-get-diff-lines (file-orig file-new) + "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=:' +which can be passed directly to 'clang-format'" + ;; 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" + ;; Printout changes as only the line groups. + "--changed-group-format=--lines=%dF:%dL " + ;; Ignore unchanged content. + "--unchanged-group-format=" + file-orig + file-new + ) + ) + (stderr (concat (if (zerop (buffer-size)) "" ": ") + (buffer-substring-no-properties + (point-min) (line-end-position) + (when (stringp status) ideasman42 wrote: \*picky\* I think a `(cond ...)` would read better than unless & if statements. example: ```elisp (cond ((stringp status) ...snip...) ((= status 0) nil) ((= status 1) ...snip...) (t (error ...snip...))) ``` https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -205,14 +288,60 @@ uses the function `buffer-file-name'." (delete-file temp-file) (when (buffer-name temp-buffer) (kill-buffer temp-buffer) +;;;###autoload +(defun clang-format-git-diffs (&optional style assume-file-name) + "The same as 'clang-format-buffer' but only operates on the git +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-git-head + (clang-format--git-diffs-get-git-head-file)) +(tmpfile-curbuf (make-temp-file "clang-format-git-tmp"))) ideasman42 wrote: It looks like this temporary file isn't removed either. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -205,14 +288,60 @@ uses the function `buffer-file-name'." (delete-file temp-file) (when (buffer-name temp-buffer) (kill-buffer temp-buffer) +;;;###autoload +(defun clang-format-git-diffs (&optional style assume-file-name) ideasman42 wrote: A more general point, would it make sense to call this `clang-format-vc-diffs` - which currently only supports git? This way we wouldn't need `clang-format-hg-diffs` or separate commands to support other version control systems in the future. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,104 @@ 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-get-diff-lines (file-orig file-new) + "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=:’ +which can be passed directly to ‘clang-format’." + ;; Use temporary buffer for output of diff. + (with-temp-buffer +;; We could use diff.el:diff-no-select here. The reason we don't +;; is diff-no-select requires extra copies on the buffers which +;; induces noticeable slowdowns, especially on larger files. +(let ((status (call-process + diff-command + nil + (current-buffer) + nil + ;; Binary diff has different behaviors that we + ;; aren't interested in. + "-a" + ;; Print new lines in file-new formatted as + ;; "--lines= " + "--changed-group-format=%(N=0?:--lines=%dF:%dM )" + ;; Don't print anything for unchanged lines + "--unchanged-group-format=" ideasman42 wrote: Suggest to use diff compatible with BSD/Linux - it _could_ be worth detecting which diff is in use and using `--unchanged-group-format` as an optimization, but that could be applied as an optimization later. Or, I suppose it could be a custom setting which only defaults to true on Linux. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add "clang-format-on-save-mode" minor mode to clang-format.el (PR #104533)
ideasman42 wrote: @mydeveloperday bump, it'd be nice to get this merged if there are no problems with the PR. https://github.com/llvm/llvm-project/pull/104533 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
ideasman42 wrote: > I don't disagree these are all potential pitfalls (and there are certainly > more), I just don't see how having the diff code in a separate project > ameliorates any of them. And as stated earlier, I think it in fact > complicates them. >From a user perspective it likely just means one extra package,possibly >setting a configuration value. In a sense this PR is incomplete in that it only supports git/diff on Unix like systems. We could consider a change to the PR that makes it less implementation spesific: Use `vc-diff` to generate the diff - this abstracts away all the details of calling git or diff directly, personally I would still rather keep the functionality separate - then this can be easily integrated with `hl-diff` or other packages that already track changed lines, removing the overhead of the current method used. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
ideasman42 wrote: _Suggesting a fairly large change to this PR, although it's quite opinionated._ If I were maintaining `clang-format.el` I'd split out the logic for generating a list of changes into it's own package. Exactly how this is done is not be so important... it could for example support a custom function that generates a list of line number pairs. `clang-format-vc-diff` could depend on a custom function which defaults to nil, and would warn when the function wasn't set. e.g. `(defcustom clang-format-vc-diff-function ...)` The function can simply return an ordered list of integer pairs representing lines. Then there can be a package on MELPA `clang-format-diff` or similar and all the issues with platform compatibility can be handled there without pull requests to LLVM. --- There are a few reasons this has benefits. - The current PR misses `(require ...)` for, `vc` `vc-git` ... however requiring this is unnecessary for users who wont use the functionality. - Any bugs relating to details of different versions of git/diff/WIN32 compatibility can be handled without going via LLVM PR. - Support for other version control or other ways of generating change ranges can be handled even customized - integrated with other packages that already track changes - such as `diff-hl` could be used. - All the details of integrating diff-hl/non-git version control, platform support (proper error messages if `diff` isn't found or encounters some unknown encoding)... can be handled separately. - LLVM's clang-format.el remains minimal, users who fully format their source don't need to install the extra functionality. - Less maintenance burden for LLVM. The main down side is users who rely on this behavior need to install an additional package. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
ideasman42 wrote: > So IIUC, you are proposing a create an entirely seperate project for > "vc-diff-lines" or something like that then making that a dependency for for > `clang-format`? Is that correct? I would not depend on it - instead, it would be loosely coupled - if `clang-format-vc-diff-function` was set, it would be used. Otherwise `clang-format-vc-diff` wouldn't work (reporting the function to calculate changed lines needs to be set). --- > At least IMO, this is the other way around. Relying on external projects I > think typically creates a higher maintainer burden, particularly in this case > where I think LLVM would be the only user of the new package, so whenever > changes where needed for the diff/vc stuff, it would require coordination > with an external project. This can happen for intricate API's - in this case - a single function that returns line-ranges: `(list (int . int) ...)` is a fairly minimal interface, so I don't see separating the code out as adding overhead/burden. > Overall, I'm pretty down on this. IMO, the vc/diff functionality is pretty > specific to the use-case we have in clang-format.el and neither is complex > enough to warrant or made more convenient by having in an independent package. Fair enough - as noted, this is fairly opinionated - and I don't think your "wrong" for holding the contrary position. Keep in mind many people have been using `clang-format.el` for years without this functionality, and will continue to do so. I disagree that this change is simple though, it seems simple on face value - based on the review - using git & diff with their different versions ... possible optimizations, possible errors when they fail ... is in fact more complicated than may first seem. There are some potential bugs that could bite us: - How to handle git failing (if git doesn't know about the file... has a corrupt repository... the file could be in the middle of resolving a conflict). - What if git or diff considers the file to be a binary file. - The emacs buffers should use the encoding settings from the buffer that is being edited... do they? (I'd need to double check - at a guess - they don't seem to at the moment). This is not to attempt to shoot down the contribution, just to note that there are all sorts of cases where things can go wrong - where a feature like this turns out not to be all that simple and there are corner cases that need to be investigated/resolved. As noted, this is my suggestion to support a customizable line-range generator so `clang-format.el can be kept minimal. I'll leave the decision to others. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -206,20 +191,30 @@ which can be passed directly to ‘clang-format’." ((= 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)) - (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)) +;; Find and collect all diff lines. +;; We are matching something like: +;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@" +(let ((diff-lines-re + "^@@\s-[0-9,]+\s\\+\\([0-9]+\\)\\(,\\([0-9]+\\)\\)?\s@@$") + (index 0) + (all-lines + (buffer-substring-no-properties (point-min) (point-max ideasman42 wrote: Did you try searching within the current buffer? It's quite strange to make a copy of the whole buffer just to search it. Even if you can measure some performance improvement, I think it's reasonable to use common practices, instead of relying on performance characteristics of the current implementation. Unless there are really significant user visible advantages. Based on my own experience with ELisp, I've never seen scripts duplicate a buffer into a string to search it. Besides relying on some details of the performance of your version of emacs, subtle changes to ELisp may render this odd way of searching the buffer invalid. There is also the down side of storing the buffer twice in memory - probably it's not going to cause problems on modern systems, but it could cause some GC/performance issues with very large buffers still. Furthermore `(while (re-search-forward ...) ...)` is such a common pattern, any bottleneck here could be caused by the arguments your using ... or some other buffer setting (case sensitivity.. or other arguments that control behavior). It seems quite suspicious that there should be a significant difference. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,124 @@ 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 +(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))) + + +(defun clang-format--vc-diff-get-diff-lines (file-orig file-new) + "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=:’ +which can be passed directly to ‘clang-format’." + ;; Use temporary buffer for output of diff. + (with-temp-buffer +;; We could use diff.el:diff-no-select here. The reason we don't +;; is diff-no-select requires extra copies on the buffers which +;; induces noticeable slowdowns, especially on larger files. +(let ((status (call-process + diff-command + 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) +;; Find and collect all diff lines. +;; We are matching something like: +;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@" +(goto-char (point-min)) +(while (re-search-forward +"^@@\s-[0-9,]+\s\\+\\([0-9]+\\)\\(,\\([0-9]+\\)\\)?\s@@$" +nil +t +1) + (let ((match1 (string-to-number (match-string 1))) +(match3 (if (match-string 3) ideasman42 wrote: `\s-` uses the syntax-table, in this case I think it's reasonable to be more explicit (space-or-tab) This is a similar regex I used for the package `diff-at-point`. Not saying you \*should\* use this, but in general I find `[:blank:]` preferable when `a space or tab character` is appropriate, see: https://www.emacswiki.org/emacs/RegularExpression `` (concat "^\\(@@\\)[[:blank:]]+" ;; Previous (ignore). "-" "\\([[:digit:]]+\\),\\([[:digit:]]+\\)" "[[:blank:]]+" ;; Current (use). "\\+" "\\([[:digit:]]+\\),\\([[:digit:]]+\\)" "[[:blank:]]+@@") ``` https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,124 @@ 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 +(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))) + + +(defun clang-format--vc-diff-get-diff-lines (file-orig file-new) + "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=:’ +which can be passed directly to ‘clang-format’." + ;; Use temporary buffer for output of diff. + (with-temp-buffer +;; We could use diff.el:diff-no-select here. The reason we don't +;; is diff-no-select requires extra copies on the buffers which +;; induces noticeable slowdowns, especially on larger files. +(let ((status (call-process + diff-command + 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) +;; Find and collect all diff lines. +;; We are matching something like: +;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@" +(goto-char (point-min)) +(while (re-search-forward +"^@@\s-[0-9,]+\s\\+\\([0-9]+\\)\\(,\\([0-9]+\\)\\)?\s@@$" +nil +t +1) + (let ((match1 (string-to-number (match-string 1))) +(match3 (if (match-string 3) ideasman42 wrote: Minor point, suggest to use the result of the first `(match-string 3)` call instead of throwing the result away & calling again. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 approved this pull request. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
ideasman42 wrote: > All things considered, I think requiring `vc` is a lot less intrusive than > essentially our own `vc` wrapper Not sure what you mean exactly by a wrapper, if git/diff logic can be abstracted away - that seems a net gain. If ugly/fragile logic is required to do that - OK, it's debatable. Anyway, I was only mentioning that we could consider abstracting away the logic - if it's not practical, there is no need to go into details discussing it, although I did mail the emacs-devel mailing list to check if this might be supported: https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
ideasman42 wrote: Checking again and am still considering this patch too spesific/incomplete, checking vc's diff calls to git - they are considerably more involved than in this PR, meaning this PR will likely require follow up commits to fix problems _(see `vc-diff-internal`, inlined below for reference, it deals with EOL conversion, added files, coding systems... things this PR doesn't really handle)._ Attached a patch that allows for formatting line-ranges, the line range generation must be implemented externally. - The `clang-format-modified-fn` customizable function is used to return a list of "modified" line ranges, this can be set by 3rd party packages - VC implementation independent. - This function simply returns a list of integer pairs (line ranges). - An error is raised if the function isn't set. Patch files: - Patch on the main branch [pr-112792-update.diff.txt](https://github.com/user-attachments/files/18309256/pr-112792-update.diff.txt) - The whole file (for convenience). [clang-format.el.update.txt](https://github.com/user-attachments/files/18309260/clang-format.el.update.txt) - The git/diff logic extracted into a separate file - which would not be applied to the LLVM project, just use for testing. [clang-format-git-vc-diff.el.txt](https://github.com/user-attachments/files/18309263/clang-format-git-vc-diff.el.txt) - As mentioned earlier, calling diff can be quite involved if all corner cases are properly handled. ``` (defun vc-diff-internal (async vc-fileset rev1 rev2 &optional verbose buffer) "Report diffs between revisions REV1 and REV2 of a fileset in VC-FILESET. ASYNC non-nil means run the backend's commands asynchronously if possible. VC-FILESET should have the format described in `vc-deduce-fileset'. Output goes to the buffer BUFFER, which defaults to *vc-diff*. BUFFER, if non-nil, should be a buffer or a buffer name. Return t if the buffer had changes, nil otherwise." (unless buffer (setq buffer "*vc-diff*")) (let* ((files (cadr vc-fileset)) (messages (cons (format "Finding changes in %s..." (vc-delistify files)) (format "No changes between %s and %s" (or rev1 "working revision") (or rev2 "workfile" ;; Set coding system based on the first file. It's a kluge, ;; but the only way to set it for each file included would ;; be to call the back end separately for each file. (coding-system-for-read ;; Force the EOL conversion to be -unix, in case the files ;; to be compared have DOS EOLs. In that case, EOL ;; conversion will produce a patch file that will either ;; fail to apply, or will change the EOL format of some of ;; the lines in the patched file. (coding-system-change-eol-conversion (if files (vc-coding-system-for-diff (car files)) 'undecided) 'unix)) (orig-diff-buffer-clone (if revert-buffer-in-progress-p (clone-buffer (generate-new-buffer-name " *vc-diff-clone*") nil ;; On MS-Windows and MS-DOS, Diff is likely to produce DOS-style ;; EOLs, which will look ugly if (car files) happens to have Unix ;; EOLs. But for Git, we must force Unix EOLs in the diffs, since ;; Git always produces Unix EOLs in the parts that didn't come ;; from the file, and wants to see any CR characters when applying ;; patches. (if (and (memq system-type '(windows-nt ms-dos)) (not (eq (car vc-fileset) 'Git))) (setq coding-system-for-read (coding-system-change-eol-conversion coding-system-for-read 'dos))) (vc-setup-buffer buffer) (message "%s" (car messages)) ;; Many backends don't handle well the case of a file that has been ;; added but not yet committed to the repo (notably CVS and Subversion). ;; Do that work here so the backends don't have to futz with it. --ESR ;; ;; Actually most backends (including CVS) have options to control the ;; behavior since which one is better depends on the user and on the ;; situation). Worse yet: this code does not handle the case where ;; `file' is a directory which contains added files. ;; I made it conditional on vc-diff-added-files but it should probably ;; just be removed (or copied/moved to specific backends). --Stef. (when vc-diff-added-files (let ((filtered '()) process-file-side-effects) (dolist (file files) (if (or (file-directory-p file) (not (string= (vc-working-revision file) "0"))) (push file filtered) ;; This file is added but not yet committed; ;; there is no repository version to diff against. (if (or rev1 rev2) (error "No revision
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,125 @@ 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 +(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))) + + +(defun clang-format--vc-diff-get-diff-lines (file-orig file-new) + "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=:’ +which can be passed directly to ‘clang-format’." + ;; Use temporary buffer for output of diff. + (with-temp-buffer +;; We could use diff.el:diff-no-select here. The reason we don't +;; is diff-no-select requires extra copies on the buffers which +;; induces noticeable slowdowns, especially on larger files. +(let ((status (call-process + diff-command + 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) +;; Find and collect all diff lines. +;; We are matching something like: +;; "@@ -80 +80 @@" or "@@ -80,2 +80,2 @@" +(goto-char (point-min)) +(while (re-search-forward + "^@@[[:blank:]]-[[:digit:],]+[[:blank:]]\\+\\([[:digit:]]+\\)\\(,\\([[:digit:]]+\\)\\)?[[:blank:]]@@$" +nil +t +1) + (let ((match1 (string-to-number (match-string 1))) +(match3 (let ((match3_or_nil (match-string 3))) + (if match3_or_nil + (string-to-number match3_or_nil) +nil +(push (format + "--lines=%d:%d" + match1 + (if match3 (+ match1 match3) match1)) + diff-lines))) +(nreverse diff-lines)) + ;; Any return != 0 && != 1 indicates some level of error. + (t +(error "(diff returned unsuccessfully %s%s)" status stderr)) ideasman42 wrote: All errors should use the "clang-format: " prefix, otherwise it can be difficult to track down errors when these functions are called indirectly. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 commented: Works well on Linux, generally seems fine although I still think it's worth considering splitting out "getting-changes" into a separate shared code-base to avoid dealing with details of calling diff for different version control system here. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -216,17 +327,72 @@ uses the function `buffer-file-name'." (if incomplete-format (message "(clang-format: incomplete (syntax errors)%s)" stderr) (message "(clang-format: success%s)" stderr - (delete-file temp-file) + (ignore-errors (delete-file temp-file)) ideasman42 wrote: suggest with-demoted-errors here too, could be a function as it's used elsewhere. https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
@@ -146,18 +146,125 @@ 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 +(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))) + + +(defun clang-format--vc-diff-get-diff-lines (file-orig file-new) ideasman42 wrote: \*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). https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [emacs][clang-format] Add elisp API for clang-format on git diffs (PR #112792)
https://github.com/ideasman42 edited https://github.com/llvm/llvm-project/pull/112792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
