[PATCH] clang-format: options for spacing template argument lists

[Daniel Jasper]

Submitted in r193614. Thanks again for working on this!


On Mon, Oct 28, 2013 at 7:01 PM, Christopher Olsen <chrisaolsen at gmail.com>wrote:

> I didn't realize there were tests for parsing options.  Added and attached.
>
> If you can submit the change that would be great.
>
> Chris
>
> On Mon, Oct 28, 2013 at 11:59 AM, Daniel Jasper <djasper at google.com>
> wrote:
> >
> >
> >
> > On Mon, Oct 28, 2013 at 5:29 PM, Christopher Olsen <
> chrisaolsen at gmail.com>
> > wrote:
> >>
> >> Thanks for the feedback.  I've updated the patch:
> >>
> >> Added the flag SpacesInAngles - A<int> vs A< int >
> >> Note that LanguageStandard=Cpp03 overrides SpacesInAngles=false in the
> >> case of '>>' as in A<A<int> >
> >>
> >> While I tend to agree with you on spacing in template arguments I have
> >> 5+ million lines of code developed over 12 years in strict "pad
> >> everything" style to contend with.  Honestly, I'm hoping clang-format
> >> will eventually allow us to re-evaluate certain style choices if it
> >> gets adopted on our project.
> >
> >
> > As I said, I can perfectly understand the necessity for SpacesInAngles
> (both
> > because there are large codebase where consistency matters and that it
> is a
> > strong matter of what one is used to). So this option is totally
> reasonable.
> > Sorry if I was unclear.
> >
> > The patch looks fine except that there doesn't seem to be a test for the
> > parsing of the configuration option. That should also be done in
> > unittests/Format/FormatTests.cpp along with the tests for parsing all the
> > other options. Do you have commit access or should I submit this patch
> for
> > you?
> >
> > Cheers,
> > Daniel
> >
> >> Chris
> >>
> >> On Sun, Oct 27, 2013 at 4:32 AM, Daniel Jasper <djasper at google.com>
> wrote:
> >> > The patch itself looks fine. However, I am also slightly skeptical
> that
> >> > it
> >> > is a good general direction.
> >> >
> >> > Specifically:
> >> > - SpaceInEmptyAngles: Quite frankly, it doesn't matter enough. Nobody
> >> > really
> >> > cares (or should care about this). I'd make a decision and get on with
> >> > life.
> >> > If you need that space for your codebase, make it dependent on
> >> > SpacesInAngles.
> >> > - SpaceAfterTemplateKeyword: Same as above, it doesn't matter enough.
> We
> >> > actually had a discussion about this very early into the clang-format
> >> > development. People don't agree, but it really does not matter (no
> >> > version
> >> > is more or less readable, ..). We then actually went ahead (IIRC) to
> >> > change
> >> > all instances in the C++11 standard to do exactly what clang-format
> does
> >> > now. I'd rather have clang-format force people into a consistent
> >> > behavior
> >> > for such non-issues than add additional technical complexity to it.
> >> > - SpacesInAngles: I would personally be appalled by having to work in
> a
> >> > codebase that uses this. Constructs like "template < class T..."
> simply
> >> > look
> >> > too much like comparisons to me. However, I can see that, if you are
> >> > used to
> >> > it, switching might be hard. The overlapping of the language standard
> is
> >> > unfortunate, but I don't really see what we can do here.
> >> >
> >> > So, I am more or less fine with introducing SpacesInAngles (goes well
> >> > with
> >> > SpacesInParentheses), but I'd rather not introduce the other two (I
> know
> >> > that we have SpaceInEmptyParentheses - I also don't like that one, but
> >> > "()"
> >> > is way more common than "<>", so it matters a bit more).
> >> >
> >> > I also agree that, at the very least, we need some grouping of the
> >> > options
> >> > soon. I have not found a way to specify style options more
> intuitively.
> >> > And
> >> > to some extend, I think people having to carefully look through the
> >> > options
> >> > or accept a default is reasonable.
> >> >
> >> > Cheers,
> >> > Daniel
> >> >
> >> >
> >> > On Fri, Oct 25, 2013 at 11:26 PM, Sean Silva <silvas at purdue.edu>
> wrote:
> >> >>
> >> >>
> >> >>
> >> >>
> >> >> On Fri, Oct 25, 2013 at 4:00 PM, Christopher Olsen
> >> >> <chrisaolsen at gmail.com>
> >> >> wrote:
> >> >>>
> >> >>> I am evaluating clang-format and need control over spacing in
> template
> >> >>> argument lists to conform to our coding standards.
> >> >>>
> >> >>> I've attached a patch that adds new flags to control this spacing.
> >> >>> Unittests are also included.
> >> >>>
> >> >>> - SpacesInAngles - A<int> vs A< int >
> >> >>> - SpaceInEmptyAngles - template <> vs template < >
> >> >>> - SpaceAfterTemplateKeyword - template<typename T> vs template
> >> >>> <typename
> >> >>> T>
> >> >>>
> >> >>> Note that LanguageStandard=Cpp03 overrides SpacesInAngles=false in
> the
> >> >>> case of '>>' as in A<A<int> >
> >> >>>
> >> >>> Please let me know if there is anything I need to fix for
> submission.
> >> >>
> >> >>
> >> >> I foresee (and have already run into multiple real use cases) where
> the
> >> >> spacing before, after, in between, etc. of many different kinds of
> >> >> tokens
> >> >> needs to be tweaked. For example:
> >> >>
> >> >> * multiplicative operators don't have spaces, but additive operators
> do
> >> >> `a
> >> >> + b*c`
> >> >> * spaces before parenthesized lists in function calls `foo (bar)`
> >> >> * spaces inside the parentheses of an `if`: `if ( cond ) {`
> >> >> * "function-like" return: `return(3)`
> >> >>
> >> >> I think we should try (though it may not be realistic) to integrate
> >> >> this
> >> >> functionality in a way that covers the different use cases and makes
> >> >> them
> >> >> interact in a consistent and understandable way; otherwise we will
> just
> >> >> end
> >> >> up growing a forest of not-easy-to-discover options that don't have
> >> >> very
> >> >> good coverage of the configuration space for the next project. For
> >> >> example,
> >> >> adapting clang-format to the OP's project coding standards requires
> >> >> adding 3
> >> >> new options; is there a realistic upper bound on the number of such
> >> >> options
> >> >> that we will need in order for clang-format to support, say, 1000
> >> >> different
> >> >> projects from 50 different companies/open-source communities?
> >> >>
> >> >> One possibility that I can imagine (although I don't know how
> feasible
> >> >> it
> >> >> is) is to ship another tool (or more likely keep it under an option
> to
> >> >> clang-format) which does a "one time" analysis to determine a set of
> >> >> parameters that will conform with a given sample source file (or
> files)
> >> >> and
> >> >> emits a configuration file. This analysis could work with a larger
> (but
> >> >> very
> >> >> consistent and well-defined) "plumbing" configuration space that
> >> >> essentially
> >> >> parameterizes the "guts" of clang-format (such as
> >> >> `spaceRequiredBefore`,
> >> >> `spaceRequiredBetween`, `splitPenalty`, etc.) in a data-driven way.
> >> >>
> >> >> On the other hand, I think it has been put out there before is that
> one
> >> >> of
> >> >> the benefits of clang-format is to help "standardize" to some extent
> on
> >> >> a
> >> >> common subset of style options, and so providing too-fine-grained
> >> >> support
> >> >> for "tweaking" the output might be undesirable from such a
> perspective.
> >> >> On
> >> >> the other hand, if clang-format wants to "dominate the world", it
> can't
> >> >> impose arbitrary changes on a project's coding style. A poignant
> >> >> question is
> >> >> "is it a goal for clang-format be able to conform with
> >> >> more-or-less-arbitrary styles without requiring requiring the users
> to
> >> >> get
> >> >> involved with clang-format development?", but I can't speak as to the
> >> >> answer.
> >> >>
> >> >> Daniel, what do you think?
> >> >>
> >> >> -- Sean Silva
> >> >>
> >> >>>
> >> >>>
> >> >>> Thanks!
> >> >>> Chris
> >> >>>
> >> >>>
> >> >>> _______________________________________________
> >> >>> cfe-commits mailing list
> >> >>> cfe-commits at cs.uiuc.edu
> >> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >> >>>
> >> >>
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131029/8e565a5f/attachment.html>