Status update: I am working on the following warnings in my next relevant pull request, "-Wimplicit-function-declaration" "-Wgnu-variable-sized-type-not-at-end" "-Wuninitialized" "-Wunused-variable" "-Wunused-function" "-Wmissing-prototypes" "-Wtautological-pointer-compare" "-Wincompatible-pointer-types"
Please pick remaining warnings below: "-Wdeprecated-declarations" "-Wformat" "-Wformat-extra-args" "-Wmacro-redefined" Thanks, Hong 2016-12-05 23:35 GMT+08:00 Hong Wu <[email protected]>: > I am working on following warnings in my next relevant pull request, > "-Wimplicit-function-declaration" > "-Wgnu-variable-sized-type-not-at-end" > "-Wtypedef-redefinition" > "-Wincompatible-pointer-types" > "-Wconstant-logical-operand" > "-Wmemsize-comparison" > "-Wnull-dereference" > "-Wpointer-sign" > "-Wint-conversion" > > Please pick remaining warning types below: > "-Wformat" > "-Wformat-extra-args" > "-Wuninitialized" > "-Wunused-variable" > "-Wunused-function" > "-Wpointer-sign" > "-Wmacro-redefined" > "-Wmissing-prototypes" > "-Wdeprecated-declarations" > "-Wtautological-pointer-compare" > > Thanks, > Hong > > 2016-12-05 13:52 GMT+08:00 Hong Wu <[email protected]>: > >> 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 >>> > > > > >>> > > > >>> > > >>> > >>> >> >> >
