klimek accepted this revision.
klimek added a comment.
This revision is now accepted and ready to land.
LG. I also think it makes the code nicer by breaking out the right functions.
Thanks!
================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormat.vsct:76
+ <Strings>
+ <ButtonText>Clang Format Document</ButtonText>
+ </Strings>
----------------
amaiorano wrote:
> klimek wrote:
> > hans wrote:
> > > amaiorano wrote:
> > > > hans wrote:
> > > > > 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?
> > > > The reason I chose "Document" is that it mimics the existing
> > > > functionality in Visual Studio:
> > > > {F2661117}
> > > >
> > > > 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.
> > > 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.
> > 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?
> @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". [[
> https://github.com/Elders/VSE-FormatDocumentOnSave | Here's an example ]] of
> an extension that offers this feature, but for VS's built-in formatting.
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.
https://reviews.llvm.org/D27501
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
