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:46:37 GMT
Yes, there are several bugs actually. We should improve code coverage rate
to get runtime errors if we miss them in the compiling period. I will fix
all of existing warnings in this week. After it, I think we should open
this option. Thanks Paul for the proposal.

Hong.

2016-12-05 13:26 GMT+08:00 Paul Guo <paulguo@gmail.com>:

> 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