Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
MIME-Version: 1.0
In-Reply-To: <CAC-pSX2GkGVZLgMEqVUOYRq4h7NcbPDcZUtvQerQNiUqnxB6tA@mail.gmail.com>
References: <CAC-pSX0UyAioen5XrQ_bR2XnQ7Pux0aSuNayhPLgiDSPuM9BLA@mail.gmail.com>
 <CADZba-RezSMSQUe+FVxdnGbN=HDnf8avTQBKTNwGn2e12W-y9A@mail.gmail.com>
 <CAMvjXi8FchYOATmMqeLNqTSQbDsUvDtnX3DVnXOvN6c6EhM_2A@mail.gmail.com>
 <CAEoRBexvmEm-8SGpzduY0TrFt9XFybBqh+-qQzy8iZUc6jMjJg@mail.gmail.com>
 <CAC-pSX0WMGXYr63kUZL91PTbO9x-jFG9nyigQPPvdD7ybV8eXg@mail.gmail.com>
 <CAC-pSX35ej+EQt0eu6qFqdfqeM-PwCejefEzDDTAFcZ7H2QP-g@mail.gmail.com>
 <CAEoRBexutypcVmdSjGCbFOenAF6SmgfO0utFAXVAjcBomEFt_w@mail.gmail.com>
 <CAFaL22bCWbBDBMf=D7UsFXKhtLQeAjyFvj2E9JZsNSHD=cU_JQ@mail.gmail.com> <CAC-pSX2GkGVZLgMEqVUOYRq4h7NcbPDcZUtvQerQNiUqnxB6tA@mail.gmail.com>
From: Daniel Hecht <dhecht@cloudera.com>
Date: Thu, 18 Aug 2016 10:06:29 -0700
Message-ID: <CAFaL22bNQd0UNMqbAjD0gh1MFHzATmnqmwbRw4A8+K_h8aW1aA@mail.gmail.com>
Subject: Re: Code formatting with clang-format
To: dev@impala.incubator.apache.org
Content-Type: multipart/alternative; boundary=94eb2c07104c638599053a5b9abb
archived-at: Thu, 18 Aug 2016 17:06:36 -0000

--94eb2c07104c638599053a5b9abb
Content-Type: text/plain; charset=UTF-8

Thanks.  Randomly looking at a bunch of files, IMO the new formatting looks
fine and even more readable in most cases.

On Thu, Aug 18, 2016 at 9:54 AM, Jim Apple <jbapple@cloudera.com> wrote:

> Done: https://gerrit.cloudera.org/#/c/4046
>
> On Thu, Aug 18, 2016 at 9:46 AM, Daniel Hecht <dhecht@cloudera.com> wrote:
>
> > Is it easy to run over the whole backend? If so, why not do that and
> post a
> > gerrit review of the diff so people can spot check whatever files they
> are
> > interested in.
> >
> > On Thu, Aug 18, 2016 at 9:37 AM, Tim Armstrong <tarmstrong@cloudera.com>
> > wrote:
> >
> > > +1 to Jim's general approach.
> > >
> > > Maybe we should get a couple more people to try it out and give
> > feedback? I
> > > tried it out on a source file I was familiar way.
> > >
> > > On Tue, Aug 16, 2016 at 10:29 AM, Jim Apple <jbapple@cloudera.com>
> > wrote:
> > >
> > > > Oh, and I should note that this file, right now, only handles C++.
> > > > clang-format also works with Java, but that's future research.
> > > >
> > > > On Tue, Aug 16, 2016 at 10:09 AM, Jim Apple <jbapple@cloudera.com>
> > > wrote:
> > > > > I support incremental reformat, not bulk reformat.
> > > > >
> > > > > I think we should make this our canonical style, but I also think
> we
> > > > > should be willing to update it sometimes. I think when we do update
> > > > > it, we don't need to do a bulk reformat.
> > > > >
> > > > > On Tue, Aug 16, 2016 at 9:06 AM, Tim Armstrong <
> > > tarmstrong@cloudera.com>
> > > > wrote:
> > > > >> +1 for automating this. I think the style is pretty good even if
> it
> > > > doesn't
> > > > >> exactly match. I think it will save a lot of time wrapping lines,
> > etc.
> > > > >>
> > > > >> What is the proposed approach to putting this into use? Will we
> just
> > > > >> incrementally reformat things as they're touched with
> > > git-clang-format,
> > > > or
> > > > >> try to do a bulk reformat?
> > > > >>
> > > > >> Will this be our canonical style? I.e. if a patch author or
> reviewer
> > > > >> doesn't like what clang-format does, do we just stick with the
> > tool's
> > > > >> output for consistency.
> > > > >>
> > > > >> My preference is that we just do it incrementally and that we do
> > make
> > > it
> > > > >> our canonical style.
> > > > >>
> > > > >>
> > > > >> On Tue, Aug 16, 2016 at 12:00 AM, Alex Behm <
> alex.behm@cloudera.com
> > >
> > > > wrote:
> > > > >>
> > > > >>> +1 for abandoning some of our style idiosyncrasies in favor of
> > > > >>> easy-to-maintain automation
> > > > >>>
> > > > >>> On Mon, Aug 15, 2016 at 4:57 PM, Henry Robinson <
> > henry@cloudera.com>
> > > > >>> wrote:
> > > > >>>
> > > > >>> > I think this is great - really useful to have. There are some
> > small
> > > > >>> > deviations from our traditional style (see the review for a
> > couple
> > > of
> > > > >>> > them). They really don't bother me, and I think it's much
> better
> > to
> > > > have
> > > > >>> > automated formatting than to hang on to the position of a : in
> a
> > > > for()
> > > > >>> > statement :) But I asked Jim if he'd start a thread here to
> check
> > > if
> > > > >>> others
> > > > >>> > agree.
> > > > >>> >
> > > > >>> > On 15 August 2016 at 15:18, Jim Apple <jbapple@cloudera.com>
> > > wrote:
> > > > >>> >
> > > > >>> > > I would like to have a clang-format config file in our
> > directory
> > > to
> > > > >>> help
> > > > >>> > > new contributors understand how to format code and have a
> tool
> > to
> > > > do it
> > > > >>> > for
> > > > >>> > > them. Through the time I've been sending patches I've been
> > > > >>> accumulating a
> > > > >>> > > .clang-format file that seems to minimize the style comments
> I
> > > > get. You
> > > > >>> > can
> > > > >>> > > see it here:
> > > > >>> > >
> > > > >>> > > https://gerrit.cloudera.org/#/c/3886
> > > > >>> > >
> > > > >>> > > And you can save it and upload it to play with here:
> > > > >>> > >
> > > > >>> > > http://zed0.co.uk/clang-format-configurator/
> > > > >>> > >
> > > > >>> > > I would love to hear your thoughts.
> > > > >>> > >
> > > > >>> >
> > > > >>> >
> > > > >>> >
> > > > >>> > --
> > > > >>> > Henry Robinson
> > > > >>> > Software Engineer
> > > > >>> > Cloudera
> > > > >>> > 415-994-6679
> > > > >>> >
> > > > >>>
> > > >
> > >
> >
>

--94eb2c07104c638599053a5b9abb--