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

154

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

123

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.

157

nit: "-assume-file-name"

194

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

126

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

197

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

       (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
-using ASSUME-FILE-NAME to locate a style config file. 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 no assume-file-name is given uses
-`buffer-file-name'."
+(defun clang-format-region (start end &optional style)
+  "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))

   (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

         ;; 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 (apply #'call-process-region
-                             nil nil clang-format-executable
-                             nil `(,temp-buffer ,temp-file) nil
-                             `("-output-replacements-xml"
-                               ;; Gaurd against a nil assume-file-name.
-                               ;; If -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))))
+        (let ((status (call-process-region
+
+                       "-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)))
               (stderr (with-temp-buffer
                         (unless (zerop (cadr (insert-file-contents temp-file)))
                           (insert ": "))

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