Mailing-List: contact dev-help@hawq.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hawq.incubator.apache.org
MIME-Version: 1.0
In-Reply-To: <CAOPnHqdvycSuwYT8xqJyM7ZK7TTeh+KPDwO4WsKCQyLQLCHnDA@mail.gmail.com>
References: <CAEET0ZGDNmG0bs6bTmv0i=RmVH8y2KEQV01=EnFHOWTsFEMJaQ@mail.gmail.com>
 <CAOK2y_fuUHWwdH9ZVq+wWXt42VN3n6Bc-2QEZtFRy2MukWrARg@mail.gmail.com>
 <CAEET0ZGTAdOn5d3qe+msFBarEUmmCvh47oP+Od1Dyj2Bmhh7PQ@mail.gmail.com>
 <CAOK2y_fS9=ajkAROcHpkPDpye6eHU=aCnWX4Qv7VcTK4J1iaLw@mail.gmail.com>
 <CABYHGscO33Zo-4-8A4v2ezB+DKiA1QtbWuuKjFQf=jRhHQnWYg@mail.gmail.com>
 <CAMTypuva4E4Ukg8f_XggT67khCj2aa=X8CknhkzGOhcF-OS93A@mail.gmail.com>
 <CAEET0ZHd_AJD+ZDShM172rBwVKY2oOVPU0PUhYH_d6ZHKY1=GA@mail.gmail.com>
 <CABYHGsdRsfAsDRdd9iTUj6SfEZf-qh79Gofyc1Z94n59iTO00Q@mail.gmail.com>
 <CABQrizeYP1R4mcr3XVcmSLkx4baT+og_=csRmd93w7n1eYgtHQ@mail.gmail.com>
 <CAOPnHqf8CivArjMvPfMjmHsnVwO0iO_=Kvq9kc++wuV9KZvuVw@mail.gmail.com>
 <CABQrizf3sgxL3Z3+KSLX6SKFubN=73zpsFUtG7vcNbAh0AvLWA@mail.gmail.com>
 <CAFht1hKNkmwXdV2tVQ5o0vhE9dhBfUkdomBnwuTkbOeFTzTiXQ@mail.gmail.com>
 <CAOPnHqd3RBGu2x-Dmd_2TSXMGK4Mphz2fgN42oFkQM3VWYUg1g@mail.gmail.com> <CAOPnHqdvycSuwYT8xqJyM7ZK7TTeh+KPDwO4WsKCQyLQLCHnDA@mail.gmail.com>
From: Hong Wu <xunzhangthu@gmail.com>
Date: Thu, 8 Dec 2016 00:32:56 +0800
Message-ID: <CAOPnHqc+GngX5ygDi81mp5_EJSvGygqMh2XyYf5NbjQk1saiRg@mail.gmail.com>
Subject: Re: Warnings and indent issues in HAWQ code.
To: dev@hawq.incubator.apache.org
Content-Type: multipart/alternative; boundary=001a11c160f6c3edbb05431412b2
archived-at: Wed, 07 Dec 2016 16:33:11 -0000

--001a11c160f6c3edbb05431412b2
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Status update:
I am working on the following warnings in my next relevant pull request,
"-Wimplicit-function-declaration"
"-Wgnu-variable-sized-type-not-at-end"
"-Wuninitialized"
"-Wunused-variable"
"-Wunused-function"
"-Wmissing-prototypes"
"-Wtautological-pointer-compare"
"-Wincompatible-pointer-types"

Please pick remaining warnings below:
"-Wdeprecated-declarations"
"-Wformat"
"-Wformat-extra-args"
"-Wmacro-redefined"

Thanks,
Hong

2016-12-05 23:35 GMT+08:00 Hong Wu <xunzhangthu@gmail.com>:

> I am working on following warnings in my next relevant pull request,
> "-Wimplicit-function-declaration"
> "-Wgnu-variable-sized-type-not-at-end"
> "-Wtypedef-redefinition"
> "-Wincompatible-pointer-types"
> "-Wconstant-logical-operand"
> "-Wmemsize-comparison"
> "-Wnull-dereference"
> "-Wpointer-sign"
> "-Wint-conversion"
>
> Please pick remaining warning types below:
> "-Wformat"
> "-Wformat-extra-args"
> "-Wuninitialized"
> "-Wunused-variable"
> "-Wunused-function"
> "-Wpointer-sign"
> "-Wmacro-redefined"
> "-Wmissing-prototypes"
> "-Wdeprecated-declarations"
> "-Wtautological-pointer-compare"
>
> Thanks,
> Hong
>
> 2016-12-05 13:52 GMT+08:00 Hong Wu <xunzhangthu@gmail.com>:
>
>> Ok, if we work together, that's sgreat! The remaining warning options
>> include:
>>
>> "-Wuninitialized"
>> "-Wtypedef-redefinition"
>> "-Wunused-variable"
>> "-Wunused-function"
>> "-Wgnu-variable-sized-type-not-at-end"
>> "-Wmissing-prototypes"
>> "-Wdeprecated-declarations"
>> "-Wmacro-redefined"
>> "-Wformat"
>> "-Wimplicit-function-declaration"
>> "-Wtautological-pointer-compare"
>> "-Wincompatible-pointer-types"
>> "-Wformat-extra-args"
>> "-Wconstant-logical-operand"
>> "-Wmemsize-comparison"
>> "-Wpointer-sign"
>> "-Wnumm-dereference"
>> "-Wpointer-sign"
>> "-Wint-conversion"
>>
>> If anybody picks some of them, please reply to this thread to avoid
>> repeated work. Thanks.
>>
>>
>> 2016-12-05 13:44 GMT+08:00 Lili Ma <lma@pivotal.io>:
>>
>>> I reviewed the PR too.
>>>
>>> This work is very important.  Sometimes it reflects as a warning, but
>>> actually it's a potential bug which is not covered by our tests.
>>>
>>> Since there are a number of work there, let's work together to fix the
>>> warnings.
>>>
>>> Thanks Paul for heading up and thanks for Hong's work!
>>>
>>> Thanks
>>> Lili
>>>
>>>
>>> On Mon, Dec 5, 2016 at 1:26 PM, Paul Guo <paulguo@gmail.com> wrote:
>>>
>>> > I just reviewed one of the warning fix related PRs which come from
>>> Hong.
>>> >
>>> > https://github.com/apache/incubator-hawq/pull/1036
>>> >
>>> > it catches several obvious bugs. I guess applying -Werror and static
>>> code
>>> > check will keep helping the code quality of HAWQ.
>>> >
>>> > Thanks for Hong's work.
>>> >
>>> >
>>> > 2016-12-01 15:37 GMT+08:00 Hong Wu <xunzhangthu@gmail.com>:
>>> >
>>> > > Thanks Paul to point it out. I will start fixing these warnings as
>>> > possible
>>> > > as I can!
>>> > >
>>> > > Hong
>>> > >
>>> > > 2016-12-01 14:17 GMT+08:00 Paul Guo <paulguo@gmail.com>:
>>> > >
>>> > > > I'm proposing the Macros below at first to help eliminate some
>>> "unused"
>>> > > > variable/argument warnings.
>>> > > >
>>> > > > Typical cases:
>>> > > >
>>> > > > 1) Variable is only used with some configurations, etc. val for
>>> > > > Assert(val). Then you could add the code below
>>> > > >     to eliminate the warning when assert is not enabled.
>>> > > >
>>> > > >    POSSIBLE_UNUSED_VAR(val);
>>> > > >
>>> > > > 2) For argument that is explicitly unused but might be kept for
>>> > > > compatibility, you could use UNUSED_ARG().
>>> > > >
>>> > > > [pguo@host67:/data2/github/incubator-hawq-a/src/include]$ git dif=
f
>>> > > > diff --git a/src/include/postgres.h b/src/include/postgres.h
>>> > > > index 1138f20..9391d6b 100644
>>> > > > --- a/src/include/postgres.h
>>> > > > +++ b/src/include/postgres.h
>>> > > > @@ -513,6 +513,18 @@ extern void gp_set_thread_sigmasks(void);
>>> > > >
>>> > > >  extern void OnMoveOutCGroupForQE(void);
>>> > > >
>>> > > > +#ifndef POSSIBLE_UNUSED_VAR
>>> > > > +#define POSSIBLE_UNUSED_VAR(x) ((void)x)
>>> > > > +#endif
>>> > > > +
>>> > > > +#ifndef POSSIBLE_UNUSED_ARG
>>> > > > +#define POSSIBLE_UNUSED_ARG(x) ((void)x)
>>> > > > +#endif
>>> > > > +
>>> > > > +#ifndef UNUSED_ARG
>>> > > > +#define UNUSED_ARG(x)          ((void)x)
>>> > > > +#endif
>>> > > > +
>>> > > >  #ifdef __cplusplus
>>> > > >  }   /* extern "C" */
>>> > > >  #endif
>>> > > >
>>> > > > 2016-12-01 10:26 GMT+08:00 Yandong Yao <yyao@pivotal.io>:
>>> > > >
>>> > > > > Great, +dev@hawq.incubator.apache.org
>>> > > > >
>>> > > > > On Thu, Dec 1, 2016 at 9:54 AM, Paul Guo <pguo@pivotal.io>
>>> wrote:
>>> > > > >
>>> > > > >> Yes, strongly agree.
>>> > > > >>
>>> > > > >> By the way, We should keep updating your story progress on
>>> Apache
>>> > JIRA
>>> > > > if
>>> > > > >> your story is not confidential.
>>> > > > >>
>>> > > > >> We should try to avoid closing an Apache JIRA with just a simp=
le
>>> > > > >> introduction, i.e. without analysis, without progress, without
>>> root
>>> > > > cause.
>>> > > > >>
>>> > > > >> On Thu, Dec 1, 2016 at 9:13 AM, Ruilong Huo <rhuo@pivotal.io>
>>> > wrote:
>>> > > > >>
>>> > > > >>> Strong +1 to move discussions about features, improvements,
>>> usages,
>>> > > etc
>>> > > > >>> in dev@hawq.incubator.apache.org and
>>> > user@hawq.incubator.apache.org
>>> > > > >>> respectively! This improve interactions between us and
>>> developers
>>> > and
>>> > > > user,
>>> > > > >>> which in turn helps to grow hawq community in apache open
>>> source
>>> > > > society.
>>> > > > >>>
>>> > > > >>> =E2=80=8B
>>> > > > >>>
>>> > > > >>> Best regards,
>>> > > > >>> Ruilong Huo
>>> > > > >>>
>>> > > > >>> On Thu, Dec 1, 2016 at 8:59 AM, Yandong Yao <yyao@pivotal.io>
>>> > wrote:
>>> > > > >>>
>>> > > > >>>> Thanks Paul for initiating this discussion, +1 for -Werror a=
nd
>>> > using
>>> > > > >>>> pgindent.
>>> > > > >>>>
>>> > > > >>>> Besides, could we move such discussion to
>>> > > > dev@hawq.incubator.apache.org,
>>> > > > >>>> it is perfect topic to discuss in open source community!
>>> > > > >>>>
>>> > > > >>>> On Wed, Nov 30, 2016 at 5:58 PM, Hong Wu <hwu@pivotal.io>
>>> wrote:
>>> > > > >>>>
>>> > > > >>>>> If we want to do this, I recommend to write the coding styl=
e
>>> > > inside a
>>> > > > >>>>> hawq.vim and a hawq.emacs file for developers to use. I agr=
ee
>>> > with
>>> > > > you that
>>> > > > >>>>> to fix compile warnings and coding styles including indents=
,
>>> we
>>> > do
>>> > > > this
>>> > > > >>>>> together with some bug fix which could minimize our effort.
>>> > > > >>>>>
>>> > > > >>>>> Hong
>>> > > > >>>>>
>>> > > > >>>>> On Wed, Nov 30, 2016 at 5:46 PM, Paul Guo <pguo@pivotal.io>
>>> > wrote:
>>> > > > >>>>>
>>> > > > >>>>>> Sorry. I meant -Werror (Which means "warning as error").
>>> > > > >>>>>>
>>> > > > >>>>>> I did not tried pgindent. Anyone knows this whether works
>>> for
>>> > us.
>>> > > By
>>> > > > >>>>>> the way, even this works I suspect there will be tons of
>>> > warnings
>>> > > > also then
>>> > > > >>>>>> we have to leave this to be fixed along with other fixes.
>>> > > > >>>>>>
>>> > > > >>>>>> Besides, pg have a coding style doc but that is very
>>> simple. I
>>> > saw
>>> > > > at
>>> > > > >>>>>> least our naming conventions are not uniform. This will be
>>> an
>>> > more
>>> > > > annoying
>>> > > > >>>>>> issue.
>>> > > > >>>>>>
>>> > > > >>>>>> On Wed, Nov 30, 2016 at 5:29 PM, Hong Wu <hwu@pivotal.io>
>>> > wrote:
>>> > > > >>>>>>
>>> > > > >>>>>>> There is no harm to fix compile warnings. Just do it, pau=
l!
>>> > > > >>>>>>>
>>> > > > >>>>>>> In "enforce -Werror (if gcc) in hawq" thread of dev maili=
ng
>>> > list,
>>> > > > >>>>>>> we talked about opening "-Werror" but there were some
>>> concerns
>>> > > > about
>>> > > > >>>>>>> the portability issue. I think it's better to fix compile
>>> > > warnings
>>> > > > but do
>>> > > > >>>>>>> not open this option.
>>> > > > >>>>>>>
>>> > > > >>>>>>> For indent and code style, HAWQ contains a tool for check
>>> that
>>> > at
>>> > > > >>>>>>> "src/tools/pgindent" which is forked from original
>>> Postgres. I
>>> > > > think we
>>> > > > >>>>>>> should run the tool to check existing code and do some
>>> indent
>>> > > > fixes.
>>> > > > >>>>>>> Meanwhile, we should also use this tool to check our
>>> c-coding
>>> > > > indent before
>>> > > > >>>>>>> sending pull requests and reviewer should pay attention t=
o
>>> > indent
>>> > > > too.
>>> > > > >>>>>>>
>>> > > > >>>>>>> Best,
>>> > > > >>>>>>> Hong
>>> > > > >>>>>>>
>>> > > > >>>>>>> On Wed, Nov 30, 2016 at 5:00 PM, Paul Guo <pguo@pivotal.i=
o
>>> >
>>> > > wrote:
>>> > > > >>>>>>>
>>> > > > >>>>>>>> Hi team,
>>> > > > >>>>>>>>
>>> > > > >>>>>>>> Long ago, I proposed to add "-Wall" (warning as error)
>>> option
>>> > in
>>> > > > >>>>>>>> the community and later found that is a big effort since
>>> > > warnings
>>> > > > are too
>>> > > > >>>>>>>> many, but I think the direction is right. I think we
>>> should
>>> > try
>>> > > > to fix
>>> > > > >>>>>>>> those warnings when you are modifying related source fil=
e,
>>> > > > besides, I found
>>> > > > >>>>>>>> indent issue is a bit severe, so if you encountered this
>>> > during
>>> > > > bug fixes
>>> > > > >>>>>>>> please fix them. All of these will improve the confidenc=
e
>>> of
>>> > > > users and
>>> > > > >>>>>>>> customers.
>>> > > > >>>>>>>>
>>> > > > >>>>>>>> Thanks.
>>> > > > >>>>>>>>
>>> > > > >>>>>>>
>>> > > > >>>>>>>
>>> > > > >>>>>>
>>> > > > >>>>>
>>> > > > >>>>
>>> > > > >>>>
>>> > > > >>>> --
>>> > > > >>>> Best Regards,
>>> > > > >>>> Yandong
>>> > > > >>>>
>>> > > > >>>
>>> > > > >>>
>>> > > > >>
>>> > > > >
>>> > > > >
>>> > > > > --
>>> > > > > Best Regards,
>>> > > > > Yandong
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>
>>
>

--001a11c160f6c3edbb05431412b2--