Submitted in r193614. Thanks again for working on this!
On Mon, Oct 28, 2013 at 7:01 PM, Christopher Olsen <[email protected]>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 <[email protected]> > wrote: > > > > > > > > On Mon, Oct 28, 2013 at 5:29 PM, Christopher Olsen < > [email protected]> > > 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 <[email protected]> > 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 <[email protected]> > wrote: > >> >> > >> >> > >> >> > >> >> > >> >> On Fri, Oct 25, 2013 at 4:00 PM, Christopher Olsen > >> >> <[email protected]> > >> >> 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 > >> >>> [email protected] > >> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >> >>> > >> >> > >> > > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
