hawq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Guo <paul...@gmail.com>
Subject Re: Warnings and indent issues in HAWQ code.
Date Mon, 05 Dec 2016 05:26:31 GMT
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