hawq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hong Wu <xunzhang...@gmail.com>
Subject Re: Warnings and indent issues in HAWQ code.
Date Wed, 07 Dec 2016 16:32:56 GMT
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
diff
>>> > > > 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 simple
>>> > > > >> 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.
>>> > > > >>>
>>> > > > >>> ‚Äč
>>> > > > >>>
>>> > > > >>> 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 and
>>> > 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 style
>>> > > inside a
>>> > > > >>>>> hawq.vim and a hawq.emacs file for developers
to use. I agree
>>> > 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, paul!
>>> > > > >>>>>>>
>>> > > > >>>>>>> In "enforce -Werror (if gcc) in hawq"
thread of dev mailing
>>> > 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 to
>>> > indent
>>> > > > too.
>>> > > > >>>>>>>
>>> > > > >>>>>>> Best,
>>> > > > >>>>>>> Hong
>>> > > > >>>>>>>
>>> > > > >>>>>>> On Wed, Nov 30, 2016 at 5:00 PM,
Paul Guo <pguo@pivotal.io
>>> >
>>> > > 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 file,
>>> > > > 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 confidence
>>> of
>>> > > > users and
>>> > > > >>>>>>>> customers.
>>> > > > >>>>>>>>
>>> > > > >>>>>>>> Thanks.
>>> > > > >>>>>>>>
>>> > > > >>>>>>>
>>> > > > >>>>>>>
>>> > > > >>>>>>
>>> > > > >>>>>
>>> > > > >>>>
>>> > > > >>>>
>>> > > > >>>> --
>>> > > > >>>> Best Regards,
>>> > > > >>>> Yandong
>>> > > > >>>>
>>> > > > >>>
>>> > > > >>>
>>> > > > >>
>>> > > > >
>>> > > > >
>>> > > > > --
>>> > > > > Best Regards,
>>> > > > > Yandong
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message