Fix assume-filename handling in clang-format.el

[werbitt]

Summary

When 'buffer-file-name' is nil 'call-process-region' returned a segmentation fault error.

This was a problem when using clang-format-buffer on an orgmode source code editing buffer.

I fixed this problem by excluding the '-assume-filename' argument when 'buffer-file-name' is nil.

To make it a bit more flexible I also added an optional argument, 'assume-file-name', to specify an assume-filename that overrides 'buffer-file-name'.

[klimek]

+Philipp, who is our emacs expert :)

[phst]

Thanks, generally looks good, but a couple of notes:

• This is actually a bug in clang-format, which can easily be triggered with clang-format -output-replacements-xml -assume-filename '' -offset 0 -length 10 -cursor 5 <<< 'aaaa = 1234;'
  
  It's fine to work around bugs, but please file a bug against clang-format itself and reference it in a comment.

• Not sure whether the assume-filename parameter is actually needed. If there's no known use case, please don't add it ("YAGNI").

• Prefer quoting function symbols with #': #'call-process-region. The byte-compiler will then warn if such a symbol is not defined as a function.

• It's more readable to write the non-varying arguments to call-process-region as direct arguments to apply, like so: (apply #'call-process-region nil ... nil (append '("-output-replacements .... The result is equivalent, but this structure makes it more obvious which arguments are positional and which are rest arguments.

• You can combine the various lists with backquotes and get rid of append: `("-output-replacements-xml" ,@(and buffer-file-name (list (concat "-assume-filename=" buffer-file-name))) "-style" ...)

[werbitt]

Clean up call-process-region

• Quote call-process-region with #', this will cause a compile time error if call-process-region is undefined
• Pass positional arguments normally (exclude from the list)
• Instead of using append use a backquoted list
• Use ,@ to splice the assume file logic into the list
• Use 'and' instead of 'if'

[werbitt]

Hi,

Thank you very much for your feedback.

I submitted a bug here:
https://bugs.llvm.org/show_bug.cgi?id=34667

I made the changes you suggested, but I left the assume-filename optional argument for now.

My current use-case is that when I'm editing a source block in an orgmode file, the code is in new buffer that doesn't have a buffer-file-name. If I'm able to pass in the base orgmode buffer-file-name, I can apply the .clang-format file from the correct directory. Without it, I'll need to locate the .clang-format file and use it to populate the style argument, which would be a lot of extra code on my end.

If you still think it's not worth it to have the assume-file argument let me know and I'll remove it.

Thanks,
Micah

[phst]

tools/clang-format/clang-format.el

125

Please document the ASSUME-FILE parameter.

187

Nit: consider renaming the parameter to ASSUME-FILENAME so that it's clear that it's a filename. Same above.

188

Please document the new parameter here as well.

[phst]

Your use case sounds good to me. Please be sure to document the new parameter, though.

tools/clang-format/clang-format.el

153

Please add a comment referencing the LLVM bug you've just filed so that others aren't surprised about this.

[werbitt]

Rename assume-file to assume-file-name and update documentation

[phst]

tools/clang-format/clang-format.el

126

Please stick to the canonical format, i.e. the first line should be a complete sentence. See https://www.gnu.org/software/emacs/manual/html_node/elisp/Documentation-Tips.html. You can use M-x checkdoc to detect such style issues automatically.
Here I'd just leave the docstring intact and add another sentence describing ASSUME-FILE-NAME at the end.

150

nit: "-assume-file-name"

189

Same here, please make the first line a complete sentence.

[werbitt]

• The first line of of an emacs docstring should be a complete sentence.
• Specify that "-assume-filname" is a clang-format option

[werbitt]

This comment was removed by werbitt.

[werbitt]

• The first line of of an emacs docstring should be a complete sentence.
• Specify that "-assume-filname" is a clang-format option

[phst]

tools/clang-format/clang-format.el

122

Hmm, somehow your patch got reverted?

[werbitt]

Thanks, I was too hasty when I tried to fix the mess I created when I diffed against an updated master and uploaded it without noticing. I believe this diff gets me back to where I intended.

[phst]

tools/clang-format/clang-format.el

125

nit: write arguments in docstrings always in caps: ASSUME-FILE-NAME

189

Write the argument in the docstring as ASSUME-FILE-NAME

[werbitt]

Use uppercase when referring to arguments

[phi]

Hi, do you need anything more from us? This patch looks fine and can be submitted.

[phi]

The patch doesn't apply any more to upstream head:

$ arc patch D37903
patching file tools/clang-format/clang-format.el
Hunk #1 FAILED at 122.
Hunk #2 FAILED at 193.

Could you please rebase it?

[werbitt]

Updating for upstream head

tools/clang-format/clang-format.el

 
 (defalias 'clang-format--filepos-to-bufferpos
   (if (fboundp 'filepos-to-bufferpos)
       'filepos-to-bufferpos
     (lambda (byte &optional _quality _coding-system)
       (byte-to-position (1+ byte)))))
 
 ;;;###autoload
-(defun clang-format-region (start end &optional style)
+(defun clang-format-region (start end &optional style assume-file)
   "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 active region.  If no style is given uses `clang-format-style'."
   (interactive
    (if (use-region-p)
        (list (region-beginning) (region-end))
      (list (point) (point))))
 
   (unless style
     (setq style clang-format-style))
 
+  (unless assume-file
+    (setq assume-file buffer-file-name))
+
   (let ((file-start (clang-format--bufferpos-to-filepos start 'approximate
                                                         'utf-8-unix))
         (file-end (clang-format--bufferpos-to-filepos end 'approximate
                                                       'utf-8-unix))
         (cursor (clang-format--bufferpos-to-filepos (point) 'exact 'utf-8-unix))
         (temp-buffer (generate-new-buffer " *clang-format-temp*"))
         (temp-file (make-temp-file "clang-format"))
         ;; Output is XML, which is always UTF-8.  Input encoding should match
         ;; the encoding used to convert between buffer and file positions,
         ;; otherwise the offsets calculated above are off.  For simplicity, we
         ;; always use ‘utf-8-unix’ and ignore the buffer coding system.
         (default-process-coding-system '(utf-8-unix . utf-8-unix)))
     (unwind-protect
-        (let ((status (call-process-region
-                       nil nil clang-format-executable
-                       nil `(,temp-buffer ,temp-file) nil
-
-                       "-output-replacements-xml"
-                       "-assume-filename" (or (buffer-file-name) "")
-                       "-style" style
-                       "-offset" (number-to-string file-start)
-                       "-length" (number-to-string (- file-end file-start))
-                       "-cursor" (number-to-string cursor)))
+        (let ((status (apply 'call-process-region
+                             (append `(nil nil ,clang-format-executable
+                                           nil (,temp-buffer ,temp-file) nil)
+                                     '("-output-replacements-xml")
+                                     (if assume-file
+                                         `("-assume-filename" ,assume-file) nil)
+                                     `("-style" ,style
+                                       "-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)))
                           (insert ": "))
                         (buffer-substring-no-properties
                          (point-min) (line-end-position)))))
           (cond
            ((stringp status)
             (error "(clang-format killed by signal %s%s)" status stderr))
(unwind-protect
                                                              'utf-8-unix)))
             (if incomplete-format
                 (message "(clang-format: incomplete (syntax errors)%s)" stderr)
               (message "(clang-format: success%s)" stderr))))
       (delete-file temp-file)
       (when (buffer-name temp-buffer) (kill-buffer temp-buffer)))))
 
 ;;;###autoload
-(defun clang-format-buffer (&optional style)
+(defun clang-format-buffer (&optional style assume-file)
   "Use clang-format to format the current buffer according to STYLE."
   (interactive)
-  (clang-format-region (point-min) (point-max) style))
+  (clang-format-region (point-min) (point-max) style assume-file))
 
 ;;;###autoload
 (defalias 'clang-format 'clang-format-region)
 
 (provide 'clang-format)
 ;;; clang-format.el ends here