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 <[email protected]>: > 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 <[email protected]>: > > > 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 <[email protected]>: > > > > > 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 <[email protected]>: > > > > > > > Great, [email protected] > > > > > > > > On Thu, Dec 1, 2016 at 9:54 AM, Paul Guo <[email protected]> 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 <[email protected]> > wrote: > > > >> > > > >>> Strong +1 to move discussions about features, improvements, usages, > > etc > > > >>> in [email protected] and > [email protected] > > > >>> 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 <[email protected]> > wrote: > > > >>> > > > >>>> Thanks Paul for initiating this discussion, +1 for -Werror and > using > > > >>>> pgindent. > > > >>>> > > > >>>> Besides, could we move such discussion to > > > [email protected], > > > >>>> it is perfect topic to discuss in open source community! > > > >>>> > > > >>>> On Wed, Nov 30, 2016 at 5:58 PM, Hong Wu <[email protected]> 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 <[email protected]> > 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 <[email protected]> > 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 <[email protected]> > > 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 > > > > > > > > > >
