This is an archive of the discontinued LLVM Phabricator instance.

Fix assume-filename handling in clang-format.el
ClosedPublic

Authored by werbitt on Sep 15 2017, 7:39 AM.

Details

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'.

Diff Detail

Event Timeline

werbitt created this revision.Sep 15 2017, 7:39 AM
klimek added a reviewer: phst.Sep 18 2017, 2:13 AM
klimek edited edge metadata.

+Philipp, who is our emacs expert :)

phst edited edge metadata.Sep 18 2017, 9:54 AM

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 updated this revision to Diff 115827.Sep 19 2017, 5:11 AM

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'

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 added inline comments.Sep 19 2017, 6:47 AM
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 accepted this revision.Sep 19 2017, 6:50 AM

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.

This revision is now accepted and ready to land.Sep 19 2017, 6:50 AM
werbitt updated this revision to Diff 115875.Sep 19 2017, 11:47 AM
werbitt edited the summary of this revision. (Show Details)

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

phst added inline comments.Sep 24 2017, 10:52 AM
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.

phst accepted this revision.Sep 24 2017, 10:52 AM
werbitt updated this revision to Diff 116601.Sep 25 2017, 1:22 PM
  • The first line of of an emacs docstring should be a complete sentence.
  • Specify that "-assume-filname" is a clang-format option
werbitt abandoned this revision.Sep 25 2017, 1:39 PM
werbitt reclaimed this revision.Sep 25 2017, 1:46 PM
This comment was removed by werbitt.
This revision is now accepted and ready to land.Sep 25 2017, 1:46 PM
werbitt updated this revision to Diff 116605.EditedSep 25 2017, 1:59 PM
  • The first line of of an emacs docstring should be a complete sentence.
  • Specify that "-assume-filname" is a clang-format option
phst added inline comments.Sep 29 2017, 11:38 AM
tools/clang-format/clang-format.el
122

Hmm, somehow your patch got reverted?

werbitt updated this revision to Diff 117500.Oct 3 2017, 4:57 AM

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 accepted this revision.Oct 3 2017, 5:36 AM
phst added inline comments.
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 updated this revision to Diff 117574.Oct 3 2017, 1:21 PM

Use uppercase when referring to arguments

phst accepted this revision.Oct 8 2017, 5:11 AM
phi added a subscriber: phi.Nov 20 2017, 4:25 AM

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

phi added a comment.Nov 20 2017, 5:04 AM

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 updated this revision to Diff 124242.Nov 24 2017, 3:13 PM
werbitt edited the summary of this revision. (Show Details)

Updating for upstream head

phi accepted this revision.Dec 2 2017, 1:12 PM
phst accepted this revision.Dec 2 2017, 1:12 PM
This revision was automatically updated to reflect the committed changes.