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 <[email protected]>: > 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 <[email protected]> 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 <[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 > > > > > > > > > > > > > > >
