Thread: Significant oversight in that #include-removal script
I just noticed that optimizer/cost.h is not #include'd by plancat.c,
which is not too cool because the former has the extern declaration
for the constraint_exclusion global variable while the latter has
the actual definition. I didn't run it down in the CVS history to
make sure, but I imagine what happened is that your unnecessary-includes
script diked it out because the file still compiled warning-free without
that header, ie, there is no warning for "int foo;" not preceded by
"extern int foo;". This isn't real good because it would allow a global
variable to get out of sync with its declaration. Is there a way to
prevent such problems in future?
regards, tom lane
Tom Lane wrote: > I just noticed that optimizer/cost.h is not #include'd by plancat.c, > which is not too cool because the former has the extern declaration > for the constraint_exclusion global variable while the latter has > the actual definition. I didn't run it down in the CVS history to > make sure, but I imagine what happened is that your unnecessary-includes > script diked it out because the file still compiled warning-free without > that header, ie, there is no warning for "int foo;" not preceded by > "extern int foo;". This isn't real good because it would allow a global > variable to get out of sync with its declaration. Is there a way to > prevent such problems in future? The script certainly has no way to know it is missing an extern, and I am not sure how I would even teach it that trick. The example you saw was: src/include/optimizer/cost.h:55:extern bool constraint_exclusion; src/backend/optimizer/util/plancat.c:46:bool constraint_exclusion= false; The only clean way I can think of to fix this would be to have all the globals in a single C file that is included as part of postgres.h. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > The script certainly has no way to know it is missing an extern, and I > am not sure how I would even teach it that trick. > > The example you saw was: > > src/include/optimizer/cost.h:55:extern bool constraint_exclusion; > src/backend/optimizer/util/plancat.c:46:bool constraint_exclusion = false; > > The only clean way I can think of to fix this would be to have all the > globals in a single C file that is included as part of postgres.h. Agreed, it seems pretty difficult. Another insane idea is to grep all the header files and make sure that no header contains an identically named variable to a variable not marked static. It would be easy if the compiler were to have an option to throw a warning when it finds a non-static variable that doesn't have a corresponding extern declaration. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Bruce Momjian wrote:
>> The script certainly has no way to know it is missing an extern, and I
>> am not sure how I would even teach it that trick.
> It would be easy if the compiler were to have an option to throw a
> warning when it finds a non-static variable that doesn't have a
> corresponding extern declaration.
Yeah, I think this is hopeless (or at least not worth the cost) without
compiler support --- I was just idly wondering if newer gcc's might have
such an option.
The case at hand is actually somewhat improbable, because it requires
having a header file that exports a variable but not any of the file's
functions. So maybe it's not worth worrying about anyway.
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Bruce Momjian wrote: > >> The script certainly has no way to know it is missing an extern, and I > >> am not sure how I would even teach it that trick. > > > It would be easy if the compiler were to have an option to throw a > > warning when it finds a non-static variable that doesn't have a > > corresponding extern declaration. > > Yeah, I think this is hopeless (or at least not worth the cost) without > compiler support --- I was just idly wondering if newer gcc's might have > such an option. > > The case at hand is actually somewhat improbable, because it requires > having a header file that exports a variable but not any of the file's > functions. So maybe it's not worth worrying about anyway. The only thing I could come up with was to look at the symbols exported by the object files. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Wed, Jan 07, 2009 at 05:49:12PM -0500, Tom Lane wrote: > > It would be easy if the compiler were to have an option to throw a > > warning when it finds a non-static variable that doesn't have a > > corresponding extern declaration. > > Yeah, I think this is hopeless (or at least not worth the cost) without > compiler support --- I was just idly wondering if newer gcc's might have > such an option. GNU ld has a --warn-common option which activates some warnings relating to this. It won't check the type, but it will check the size. It does note that this might produce spurious warnings due to sloppiness in libraries, but it might help. Have anice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Please line up in a tree and maintain the heap invariant while > boarding. Thank you for flying nlogn airlines.