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

124–127

Please document the ASSUME-FILE parameter.

193–194

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

194–198

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

155

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

125

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.

152

nit: "-assume-file-name"

195

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

195

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-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 active region.  If no style is given uses `clang-format-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))))
 
   (unless style
     (setq style clang-format-style))
 
+  (unless assume-file-name
+    (setq assume-file-name 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
+        (let ((status (apply #'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)))
+                             `("-output-replacements-xml"
+                               ;; Gaurd against a nil assume-file-name.
+                               ;; If the clang-format option -assume-filename
+                               ;; is given a blank string it will crash as per
+                               ;; the following bug report
+                               ;; https://bugs.llvm.org/show_bug.cgi?id=34667
+                               ,@(and assume-file-name
+                                      (list "-assume-filename" assume-file-name))
+                               "-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
               (goto-char (clang-format--filepos-to-bufferpos cursor 'exact
                                                              '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)
-  "Use clang-format to format the current buffer according to STYLE."
+(defun clang-format-buffer (&optional style assume-file-name)
+  "Use clang-format to format the current buffer according to STYLE.
+If no STYLE is given uses `clang-format-style'. Use ASSUME-FILE-NAME
+to locate a style config file. If no ASSUME-FILE-NAME is given uses
+the function `buffer-file-name'."
   (interactive)
-  (clang-format-region (point-min) (point-max) style))
+  (clang-format-region (point-min) (point-max) style assume-file-name))
 
 ;;;###autoload
 (defalias 'clang-format 'clang-format-region)
 
 (provide 'clang-format)
 ;;; clang-format.el ends here