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 Mon, 05 Dec 2016 15:35:51 GMT
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