You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by Lars Volker <lv...@cloudera.com> on 2016/09/02 16:36:08 UTC
Request for feedback: C++ Style Guide
After some confusion in reviews about how to format code I moved our
internal C++ Style Guide wiki page to the Apache wiki and updated all links
in it. You can find it here:
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536
At some point in time someone seems to have started a list of pro and cons,
some of which are worded rather negative. Do we want to revisit those
comments or the style guide even?
I'm looking forward to any feedback. Thanks, Lars
Re: Request for feedback: C++ Style Guide
Posted by Lars Volker <lv...@cloudera.com>.
I added a paragraph describing our usage of clang-format, including prior
remarks in this thread by Tim and Jim.
I'd be glad for any feedback. Thanks, Lars
On Tue, Sep 6, 2016 at 6:55 PM, Jim Apple <jb...@cloudera.com> wrote:
> Filed https://issues.cloudera.org/browse/IMPALA-4079
>
> On Sat, Sep 3, 2016 at 7:28 PM, Todd Lipcon <to...@cloudera.com> wrote:
>
> > Agreed that the lint script is a bit hacky, but it has very few false
> > positives, mostly false negatives. We run that lint script on every patch
> > in kudu. (Though tuned to disable a few of the more nit picky rules)
> >
> > Todd
> >
> > On Sep 2, 2016 10:24 AM, "Jim Apple" <jb...@cloudera.com> wrote:
> >
> > > I'm in favor of that. It's also worth mentioning that .clang-format is
> > > about what we do going forward, and not carte blanche to send patches
> > > that reformat whitespace in files written before we had .clang-format.
> > >
> > > Some of the style guide, of course, refers to non-whitespace based
> > > rules. Google has a linter script that tries to find divergence form
> > > the standard, but I am dubious of it:
> > >
> > > https://github.com/google/styleguide/blob/gh-pages/cpplint/cpplint.py
> > >
> > > It has known places where it is inaccurate, and it doesn't understand
> the
> > > AST.
> > >
> > > On Fri, Sep 2, 2016 at 10:03 AM, Tim Armstrong <
> tarmstrong@cloudera.com>
> > > wrote:
> > > > I'm not sure that the pros and cons are that enlightening. May make
> > sense
> > > > to just remove them - not sure what others think.
> > > >
> > > > For formatting, I think we should consider leaning more on
> > clang-format.
> > > > The discussion around clang-format seemed to be going that direction.
> > > >
> > > > E.g. maybe the rule should be something like "Our .clang-format is
> the
> > > > source of truth for how to deal with whitespace, except when
> > > clang-format's
> > > > output greatly diverges from the existing code style or common sense.
> > In
> > > > that case, we should update the .clang-format file."
> > > >
> > > > On Fri, Sep 2, 2016 at 9:59 AM, Tim Armstrong <
> tarmstrong@cloudera.com
> > >
> > > > wrote:
> > > >
> > > >> Yes, several things are completely wrong. E.g. we never use c-style
> > > casts.
> > > >>
> > > >> On Fri, Sep 2, 2016 at 9:38 AM, Jim Apple <jb...@cloudera.com>
> > wrote:
> > > >>
> > > >>> I left a comment on the page - I'm not sure how much these reflect
> > our
> > > >>> actual current practice.
> > > >>>
> > > >>> On Fri, Sep 2, 2016 at 9:36 AM, Lars Volker <lv...@cloudera.com>
> wrote:
> > > >>> > After some confusion in reviews about how to format code I moved
> > our
> > > >>> > internal C++ Style Guide wiki page to the Apache wiki and updated
> > all
> > > >>> links
> > > >>> > in it. You can find it here:
> > > >>> >
> > > >>> > https://cwiki.apache.org/confluence/pages/viewpage.action?
> > > >>> pageId=65868536
> > > >>> >
> > > >>> > At some point in time someone seems to have started a list of pro
> > and
> > > >>> cons,
> > > >>> > some of which are worded rather negative. Do we want to revisit
> > those
> > > >>> > comments or the style guide even?
> > > >>> >
> > > >>> > I'm looking forward to any feedback. Thanks, Lars
> > > >>>
> > > >>
> > > >>
> > >
> >
>
Re: Request for feedback: C++ Style Guide
Posted by Jim Apple <jb...@cloudera.com>.
Filed https://issues.cloudera.org/browse/IMPALA-4079
On Sat, Sep 3, 2016 at 7:28 PM, Todd Lipcon <to...@cloudera.com> wrote:
> Agreed that the lint script is a bit hacky, but it has very few false
> positives, mostly false negatives. We run that lint script on every patch
> in kudu. (Though tuned to disable a few of the more nit picky rules)
>
> Todd
>
> On Sep 2, 2016 10:24 AM, "Jim Apple" <jb...@cloudera.com> wrote:
>
> > I'm in favor of that. It's also worth mentioning that .clang-format is
> > about what we do going forward, and not carte blanche to send patches
> > that reformat whitespace in files written before we had .clang-format.
> >
> > Some of the style guide, of course, refers to non-whitespace based
> > rules. Google has a linter script that tries to find divergence form
> > the standard, but I am dubious of it:
> >
> > https://github.com/google/styleguide/blob/gh-pages/cpplint/cpplint.py
> >
> > It has known places where it is inaccurate, and it doesn't understand the
> > AST.
> >
> > On Fri, Sep 2, 2016 at 10:03 AM, Tim Armstrong <ta...@cloudera.com>
> > wrote:
> > > I'm not sure that the pros and cons are that enlightening. May make
> sense
> > > to just remove them - not sure what others think.
> > >
> > > For formatting, I think we should consider leaning more on
> clang-format.
> > > The discussion around clang-format seemed to be going that direction.
> > >
> > > E.g. maybe the rule should be something like "Our .clang-format is the
> > > source of truth for how to deal with whitespace, except when
> > clang-format's
> > > output greatly diverges from the existing code style or common sense.
> In
> > > that case, we should update the .clang-format file."
> > >
> > > On Fri, Sep 2, 2016 at 9:59 AM, Tim Armstrong <tarmstrong@cloudera.com
> >
> > > wrote:
> > >
> > >> Yes, several things are completely wrong. E.g. we never use c-style
> > casts.
> > >>
> > >> On Fri, Sep 2, 2016 at 9:38 AM, Jim Apple <jb...@cloudera.com>
> wrote:
> > >>
> > >>> I left a comment on the page - I'm not sure how much these reflect
> our
> > >>> actual current practice.
> > >>>
> > >>> On Fri, Sep 2, 2016 at 9:36 AM, Lars Volker <lv...@cloudera.com> wrote:
> > >>> > After some confusion in reviews about how to format code I moved
> our
> > >>> > internal C++ Style Guide wiki page to the Apache wiki and updated
> all
> > >>> links
> > >>> > in it. You can find it here:
> > >>> >
> > >>> > https://cwiki.apache.org/confluence/pages/viewpage.action?
> > >>> pageId=65868536
> > >>> >
> > >>> > At some point in time someone seems to have started a list of pro
> and
> > >>> cons,
> > >>> > some of which are worded rather negative. Do we want to revisit
> those
> > >>> > comments or the style guide even?
> > >>> >
> > >>> > I'm looking forward to any feedback. Thanks, Lars
> > >>>
> > >>
> > >>
> >
>
Re: Request for feedback: C++ Style Guide
Posted by Todd Lipcon <to...@cloudera.com>.
Agreed that the lint script is a bit hacky, but it has very few false
positives, mostly false negatives. We run that lint script on every patch
in kudu. (Though tuned to disable a few of the more nit picky rules)
Todd
On Sep 2, 2016 10:24 AM, "Jim Apple" <jb...@cloudera.com> wrote:
> I'm in favor of that. It's also worth mentioning that .clang-format is
> about what we do going forward, and not carte blanche to send patches
> that reformat whitespace in files written before we had .clang-format.
>
> Some of the style guide, of course, refers to non-whitespace based
> rules. Google has a linter script that tries to find divergence form
> the standard, but I am dubious of it:
>
> https://github.com/google/styleguide/blob/gh-pages/cpplint/cpplint.py
>
> It has known places where it is inaccurate, and it doesn't understand the
> AST.
>
> On Fri, Sep 2, 2016 at 10:03 AM, Tim Armstrong <ta...@cloudera.com>
> wrote:
> > I'm not sure that the pros and cons are that enlightening. May make sense
> > to just remove them - not sure what others think.
> >
> > For formatting, I think we should consider leaning more on clang-format.
> > The discussion around clang-format seemed to be going that direction.
> >
> > E.g. maybe the rule should be something like "Our .clang-format is the
> > source of truth for how to deal with whitespace, except when
> clang-format's
> > output greatly diverges from the existing code style or common sense. In
> > that case, we should update the .clang-format file."
> >
> > On Fri, Sep 2, 2016 at 9:59 AM, Tim Armstrong <ta...@cloudera.com>
> > wrote:
> >
> >> Yes, several things are completely wrong. E.g. we never use c-style
> casts.
> >>
> >> On Fri, Sep 2, 2016 at 9:38 AM, Jim Apple <jb...@cloudera.com> wrote:
> >>
> >>> I left a comment on the page - I'm not sure how much these reflect our
> >>> actual current practice.
> >>>
> >>> On Fri, Sep 2, 2016 at 9:36 AM, Lars Volker <lv...@cloudera.com> wrote:
> >>> > After some confusion in reviews about how to format code I moved our
> >>> > internal C++ Style Guide wiki page to the Apache wiki and updated all
> >>> links
> >>> > in it. You can find it here:
> >>> >
> >>> > https://cwiki.apache.org/confluence/pages/viewpage.action?
> >>> pageId=65868536
> >>> >
> >>> > At some point in time someone seems to have started a list of pro and
> >>> cons,
> >>> > some of which are worded rather negative. Do we want to revisit those
> >>> > comments or the style guide even?
> >>> >
> >>> > I'm looking forward to any feedback. Thanks, Lars
> >>>
> >>
> >>
>
Re: Request for feedback: C++ Style Guide
Posted by Jim Apple <jb...@cloudera.com>.
I'm in favor of that. It's also worth mentioning that .clang-format is
about what we do going forward, and not carte blanche to send patches
that reformat whitespace in files written before we had .clang-format.
Some of the style guide, of course, refers to non-whitespace based
rules. Google has a linter script that tries to find divergence form
the standard, but I am dubious of it:
https://github.com/google/styleguide/blob/gh-pages/cpplint/cpplint.py
It has known places where it is inaccurate, and it doesn't understand the AST.
On Fri, Sep 2, 2016 at 10:03 AM, Tim Armstrong <ta...@cloudera.com> wrote:
> I'm not sure that the pros and cons are that enlightening. May make sense
> to just remove them - not sure what others think.
>
> For formatting, I think we should consider leaning more on clang-format.
> The discussion around clang-format seemed to be going that direction.
>
> E.g. maybe the rule should be something like "Our .clang-format is the
> source of truth for how to deal with whitespace, except when clang-format's
> output greatly diverges from the existing code style or common sense. In
> that case, we should update the .clang-format file."
>
> On Fri, Sep 2, 2016 at 9:59 AM, Tim Armstrong <ta...@cloudera.com>
> wrote:
>
>> Yes, several things are completely wrong. E.g. we never use c-style casts.
>>
>> On Fri, Sep 2, 2016 at 9:38 AM, Jim Apple <jb...@cloudera.com> wrote:
>>
>>> I left a comment on the page - I'm not sure how much these reflect our
>>> actual current practice.
>>>
>>> On Fri, Sep 2, 2016 at 9:36 AM, Lars Volker <lv...@cloudera.com> wrote:
>>> > After some confusion in reviews about how to format code I moved our
>>> > internal C++ Style Guide wiki page to the Apache wiki and updated all
>>> links
>>> > in it. You can find it here:
>>> >
>>> > https://cwiki.apache.org/confluence/pages/viewpage.action?
>>> pageId=65868536
>>> >
>>> > At some point in time someone seems to have started a list of pro and
>>> cons,
>>> > some of which are worded rather negative. Do we want to revisit those
>>> > comments or the style guide even?
>>> >
>>> > I'm looking forward to any feedback. Thanks, Lars
>>>
>>
>>
Re: Request for feedback: C++ Style Guide
Posted by Tim Armstrong <ta...@cloudera.com>.
I'm not sure that the pros and cons are that enlightening. May make sense
to just remove them - not sure what others think.
For formatting, I think we should consider leaning more on clang-format.
The discussion around clang-format seemed to be going that direction.
E.g. maybe the rule should be something like "Our .clang-format is the
source of truth for how to deal with whitespace, except when clang-format's
output greatly diverges from the existing code style or common sense. In
that case, we should update the .clang-format file."
On Fri, Sep 2, 2016 at 9:59 AM, Tim Armstrong <ta...@cloudera.com>
wrote:
> Yes, several things are completely wrong. E.g. we never use c-style casts.
>
> On Fri, Sep 2, 2016 at 9:38 AM, Jim Apple <jb...@cloudera.com> wrote:
>
>> I left a comment on the page - I'm not sure how much these reflect our
>> actual current practice.
>>
>> On Fri, Sep 2, 2016 at 9:36 AM, Lars Volker <lv...@cloudera.com> wrote:
>> > After some confusion in reviews about how to format code I moved our
>> > internal C++ Style Guide wiki page to the Apache wiki and updated all
>> links
>> > in it. You can find it here:
>> >
>> > https://cwiki.apache.org/confluence/pages/viewpage.action?
>> pageId=65868536
>> >
>> > At some point in time someone seems to have started a list of pro and
>> cons,
>> > some of which are worded rather negative. Do we want to revisit those
>> > comments or the style guide even?
>> >
>> > I'm looking forward to any feedback. Thanks, Lars
>>
>
>
Re: Request for feedback: C++ Style Guide
Posted by Jim Apple <jb...@cloudera.com>.
Done. I removed some other stuff that we don't do, either.
On Tue, Sep 6, 2016 at 8:59 AM, Marcel Kornacker <ma...@cloudera.com>
wrote:
> Who wrote the original document? There are several items on there that
> are the exact opposite of what we do (basically everything in the
> 'Classes' section).
>
> I would prefer to remove that entire section. Anyone against that?
>
> On Fri, Sep 2, 2016 at 9:59 AM, Tim Armstrong <ta...@cloudera.com>
> wrote:
> > Yes, several things are completely wrong. E.g. we never use c-style
> casts.
> >
> > On Fri, Sep 2, 2016 at 9:38 AM, Jim Apple <jb...@cloudera.com> wrote:
> >
> >> I left a comment on the page - I'm not sure how much these reflect our
> >> actual current practice.
> >>
> >> On Fri, Sep 2, 2016 at 9:36 AM, Lars Volker <lv...@cloudera.com> wrote:
> >> > After some confusion in reviews about how to format code I moved our
> >> > internal C++ Style Guide wiki page to the Apache wiki and updated all
> >> links
> >> > in it. You can find it here:
> >> >
> >> > https://cwiki.apache.org/confluence/pages/viewpage.
> >> action?pageId=65868536
> >> >
> >> > At some point in time someone seems to have started a list of pro and
> >> cons,
> >> > some of which are worded rather negative. Do we want to revisit those
> >> > comments or the style guide even?
> >> >
> >> > I'm looking forward to any feedback. Thanks, Lars
> >>
>
Re: Request for feedback: C++ Style Guide
Posted by Marcel Kornacker <ma...@cloudera.com>.
Who wrote the original document? There are several items on there that
are the exact opposite of what we do (basically everything in the
'Classes' section).
I would prefer to remove that entire section. Anyone against that?
On Fri, Sep 2, 2016 at 9:59 AM, Tim Armstrong <ta...@cloudera.com> wrote:
> Yes, several things are completely wrong. E.g. we never use c-style casts.
>
> On Fri, Sep 2, 2016 at 9:38 AM, Jim Apple <jb...@cloudera.com> wrote:
>
>> I left a comment on the page - I'm not sure how much these reflect our
>> actual current practice.
>>
>> On Fri, Sep 2, 2016 at 9:36 AM, Lars Volker <lv...@cloudera.com> wrote:
>> > After some confusion in reviews about how to format code I moved our
>> > internal C++ Style Guide wiki page to the Apache wiki and updated all
>> links
>> > in it. You can find it here:
>> >
>> > https://cwiki.apache.org/confluence/pages/viewpage.
>> action?pageId=65868536
>> >
>> > At some point in time someone seems to have started a list of pro and
>> cons,
>> > some of which are worded rather negative. Do we want to revisit those
>> > comments or the style guide even?
>> >
>> > I'm looking forward to any feedback. Thanks, Lars
>>
Re: Request for feedback: C++ Style Guide
Posted by Tim Armstrong <ta...@cloudera.com>.
Yes, several things are completely wrong. E.g. we never use c-style casts.
On Fri, Sep 2, 2016 at 9:38 AM, Jim Apple <jb...@cloudera.com> wrote:
> I left a comment on the page - I'm not sure how much these reflect our
> actual current practice.
>
> On Fri, Sep 2, 2016 at 9:36 AM, Lars Volker <lv...@cloudera.com> wrote:
> > After some confusion in reviews about how to format code I moved our
> > internal C++ Style Guide wiki page to the Apache wiki and updated all
> links
> > in it. You can find it here:
> >
> > https://cwiki.apache.org/confluence/pages/viewpage.
> action?pageId=65868536
> >
> > At some point in time someone seems to have started a list of pro and
> cons,
> > some of which are worded rather negative. Do we want to revisit those
> > comments or the style guide even?
> >
> > I'm looking forward to any feedback. Thanks, Lars
>
Re: Request for feedback: C++ Style Guide
Posted by Jim Apple <jb...@cloudera.com>.
I left a comment on the page - I'm not sure how much these reflect our
actual current practice.
On Fri, Sep 2, 2016 at 9:36 AM, Lars Volker <lv...@cloudera.com> wrote:
> After some confusion in reviews about how to format code I moved our
> internal C++ Style Guide wiki page to the Apache wiki and updated all links
> in it. You can find it here:
>
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536
>
> At some point in time someone seems to have started a list of pro and cons,
> some of which are worded rather negative. Do we want to revisit those
> comments or the style guide even?
>
> I'm looking forward to any feedback. Thanks, Lars