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 > > > > > >
