Page MenuHomePhabricator

clang-format-vsix: add command to format document
ClosedPublic

Authored by amaiorano on Dec 6 2016, 7:02 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

amaiorano updated this revision to Diff 80532.Dec 6 2016, 7:02 PM
amaiorano retitled this revision from to clang-format-vsix: add command to format document.
amaiorano updated this object.
amaiorano added reviewers: hans, zturner, klimek.
amaiorano added a subscriber: cfe-commits.
amaiorano updated this revision to Diff 80533.Dec 6 2016, 7:09 PM

My first patch accidentally included the changes from https://reviews.llvm.org/D27440

hans added inline comments.Dec 7 2016, 11:31 AM
tools/clang-format-vs/ClangFormat/ClangFormat.vsct
76 ↗(On Diff #80533)

I think File would be better than Document when referring to source code.

But it seems a little annoying to need two menu alternatives. Could we make the regular "Clang Format" option just format the whole file if there is currently no selection, or would that be confusing?

amaiorano added inline comments.Dec 7 2016, 11:44 AM
tools/clang-format-vs/ClangFormat/ClangFormat.vsct
76 ↗(On Diff #80533)

The reason I chose "Document" is that it mimics the existing functionality in Visual Studio:

As you can see, "Ctrl+K, Ctrl+F" is bound to format selection by default; which I assumed is the reason "Ctrl+R, Ctrl+F" was used for Clang Format (Selection). So along the same lines, I bound "Ctrl + R, Ctrl + D" to Clang Format Document.

I don't really have a problem with having multiple menu options, although we could have both of them underneath a "Clang Format" top menu, with "Format Selection" and "Format Document" as sub menu items.

As for annoyance in menus, I think most developers would reach for keyboard shortcuts in this case anyway. Furthermore, the next feature I want to add, building on top of this one, is allowing the user to enable "Format on Save", which would make this even easier to use.

hans added inline comments.Dec 7 2016, 1:23 PM
tools/clang-format-vs/ClangFormat/ClangFormat.vsct
76 ↗(On Diff #80533)

I see, I didn't realize they call it documents. And if they also have separate commands for formating selection and formating the whole file, maybe that makes sense for us too.

Right, I also imagine folks would use this from the keyboard, but there too it's annoying to have two shortcuts for the same thing. But again, if that's how it's generally done in VS...

I'd like to hear from Manuel on this patch too, though.

klimek added inline comments.Dec 8 2016, 2:21 AM
tools/clang-format-vs/ClangFormat/ClangFormat.vsct
76 ↗(On Diff #80533)

Clang-format currently formats the current line / statement if there is no selection, and that's how most people I know use it.
Formatting the whole file by default is not what clang-format is optimized for, and highly disruptive (eclipse does that, and it's annoying).
I'm more wondering why we need a setting for this at all - isn't ctrl-a,ctrl-k,ctrl-f equivalent to formatting the whole file?

amaiorano added a comment.EditedDec 9 2016, 9:39 AM

(NOTE, I forgot to click Submit on the reply to @klimek yesterday)

tools/clang-format-vs/ClangFormat/ClangFormat.vsct
76 ↗(On Diff #80533)

@klimek My own experience, which also seems to match those of others online, is that clang-format is a fantastic tool for making sure your code-base follows a strict formatting standard. What many people do is install Git pre-stage hooks, for instance, to run clang-format across entire source files, to ensure they're formatted properly. So even if it's not optimized for that, it works very well :)

Using clang-format to format only the current line doesn't guarantee that everything will be formatted accordingly. My own experience is that it sometimes formats the line differently than if you format the entire document. Or perhaps you end up writing a few lines of code, and formatting current line doesn't pick up all the lines you've modified. As for ctrl-a,ctrl-r-ctrl-f, the problem with it is that you lose your cursor position, as ctrl-a will place it at the end of the file.

You also mention that formatting the entire file is "highly disruptive" like in eclipse. I'm not sure how eclipse handles that, but so far VS seems to do a pretty good job. It manages to maintain breakpoint positions quite well, it manages to keep the cursor in the same place, and it manages to keep the relative location of the code where the cursor is at visible on screen. I suspect part of the reason it works well in VS is because Microsoft made sure it worked for their own format document feature (ctrl-k,ctrl-d).

As I mentioned earlier, this is a stepping stone towards adding a feature to "format on save", for which I'd like to offer the ability to choose "current line/selection" and "document". Here's an example of an extension that offers this feature, but for VS's built-in formatting.

klimek accepted this revision.Dec 12 2016, 6:54 AM
klimek edited edge metadata.

LG. I also think it makes the code nicer by breaking out the right functions. Thanks!

tools/clang-format-vs/ClangFormat/ClangFormat.vsct
76 ↗(On Diff #80533)

Note: with "highly disruptive" I meant only if the default behavior without any selection is to format the whole file, as that basically kills the workflow; that was what Hans suggested.

I'm not opposed to having an extra key binding if enough folks want it and there is enough benefit - the argument to not move your cursor is a good point.

This revision is now accepted and ready to land.Dec 12 2016, 6:54 AM
amaiorano added inline comments.Dec 12 2016, 7:33 AM
tools/clang-format-vs/ClangFormat/ClangFormat.vsct
76 ↗(On Diff #80533)

Thanks! Now I just need to get my svn submit working. Chris Latner created my account, etc. but when I tried to submit another accepted change, it didn't work. Anyway, will figure it out. Thanks again :)

This revision was automatically updated to reflect the committed changes.