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 05:52:13 GMT
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