Thread: A small rant about coding style for backend functions
I don't want to pick on Teodor in particular, because I've seen other
people do this too, but which of these functions do you find more
readable?
Datum
to_tsquery_byname(PG_FUNCTION_ARGS)
{ PG_RETURN_DATUM(DirectFunctionCall2( to_tsquery_byid,
ObjectIdGetDatum(name2cfgId((text*) PG_GETARG_POINTER(0), false)),
PG_GETARG_DATUM(1) ));
}
Datum
to_tsquery_byname(PG_FUNCTION_ARGS)
{ text *cfgname = PG_GETARG_TEXT_P(0); text *txt = PG_GETARG_TEXT_P(1); Oid cfgId;
cfgId = name2cfgId(cfgname, false); PG_RETURN_DATUM(DirectFunctionCall2(to_tsquery_byid,
ObjectIdGetDatum(cfgId), PointerGetDatum(txt)));
}
The main drawback to the V1-call-convention function call mechanism,
compared to ordinary C functions, is that you can't instantly see what
the function arguments are supposed to be. I think that good coding
style demands ameliorating this by declaring and extracting all the
arguments at the top of the function. The above example is bad enough,
but when you have to dig through a function of many lines looking for
GETARG calls in order to know what arguments it expects, it's seriously
annoying and unreadable.
And another thing: use the correct extraction macro for the argument's
type, rather than making something up on the fly. Quite aside from
helping the reader see what the function expects, the first example
above is actually *wrong*, as it will crash on toasted input.
OK, I'm done venting ... back to patch-fixing.
regards, tom lane
On 8/18/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > The main drawback to the V1-call-convention function call mechanism, > compared to ordinary C functions, is that you can't instantly see what > the function arguments are supposed to be. I think that good coding > style demands ameliorating this by declaring and extracting all the > arguments at the top of the function. The above example is bad enough, > but when you have to dig through a function of many lines looking for > GETARG calls in order to know what arguments it expects, it's seriously > annoying and unreadable. > > And another thing: use the correct extraction macro for the argument's > type, rather than making something up on the fly. Quite aside from > helping the reader see what the function expects, the first example > above is actually *wrong*, as it will crash on toasted input. This is all useful guidance. My question is why it's not part of the developer documentation. Which brings me around to a minor rant of my own. All the developer FAQ has to say about coding style is that we use 4-space tabs for indentation, and that you should "merge seamlessly into the surrounding code". That isn't much solace when the surrounding code is itself nigh unreadable or doesn't contain examples of what you are trying to do. For postgres hacking newbies (such as myself), the lack of any obvious published coding standards for the project is daunting, and is bound to lead to those developers "filling in the blanks" with their own coding style biases. Which means the patch reviewers need to spend time pointing out the flaws, and the submitter needs to spend time adjusting, testing and resubmitting ... it's all quite avoidable. I humbly suggest that if the sort of valuable information posted by Tom here was documented instead of ranted to the mailing list, maybe you guys wouldn't have to do so much ranting =) Cheers BJ
I have not forgotten this suggestion. Do have any ideas what such a list would look like? Examples? I think we have avoided more details in fear of scaring off coders. People usually follow our style as they gain experience. Having a hard list seems like it would be a lot of do's and don't's. --------------------------------------------------------------------------- Brendan Jurd wrote: > On 8/18/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > The main drawback to the V1-call-convention function call mechanism, > > compared to ordinary C functions, is that you can't instantly see what > > the function arguments are supposed to be. I think that good coding > > style demands ameliorating this by declaring and extracting all the > > arguments at the top of the function. The above example is bad enough, > > but when you have to dig through a function of many lines looking for > > GETARG calls in order to know what arguments it expects, it's seriously > > annoying and unreadable. > > > > And another thing: use the correct extraction macro for the argument's > > type, rather than making something up on the fly. Quite aside from > > helping the reader see what the function expects, the first example > > above is actually *wrong*, as it will crash on toasted input. > > This is all useful guidance. My question is why it's not part of the > developer documentation. Which brings me around to a minor rant of my > own. > > All the developer FAQ has to say about coding style is that we use > 4-space tabs for indentation, and that you should "merge seamlessly > into the surrounding code". That isn't much solace when the > surrounding code is itself nigh unreadable or doesn't contain examples > of what you are trying to do. > > For postgres hacking newbies (such as myself), the lack of any obvious > published coding standards for the project is daunting, and is bound > to lead to those developers "filling in the blanks" with their own > coding style biases. Which means the patch reviewers need to spend > time pointing out the flaws, and the submitter needs to spend time > adjusting, testing and resubmitting ... it's all quite avoidable. > > I humbly suggest that if the sort of valuable information posted by > Tom here was documented instead of ranted to the mailing list, maybe > you guys wouldn't have to do so much ranting =) > > Cheers > BJ > > ---------------------------(end of broadcast)--------------------------- > TIP 5: don't forget to increase your free space map settings -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On 11/1/07, Bruce Momjian <bruce@momjian.us> wrote: > > I have not forgotten this suggestion. Do have any ideas what such a > list would look like? Examples? > Thanks for the reply Bruce. Code examples, perhaps with "good style" and "bad style" versions to illustrate each point. In the case of Tom's OP about making all your GETARG calls at the top of the function, you could show (for a trivial function) how it looks with all the GETARGs stacked neatly at the top, and how it looks with the GETARGs scattered and inaccurate. I think that would make it immediately clear to any newbie why it's a good idea. > I think we have avoided more details in fear of scaring off coders. > People usually follow our style as they gain experience. Having a hard > list seems like it would be a lot of do's and don't's. Just my perspective, but I think you'll scare off a lot more coders by giving them no firm guidance in the first place, and then jumping down their throats with "you did this wrong" when they post a patch. Might be worth opening up a wiki page for devs to contribute their insights about writing excellent (as opposed to merely passable) Postgres code. The GETARG rant could make a good starting point. Cheers BJ
I understand your suggestions but it seems there would be too many individual items to be readable. Can you suggest a full list so we can get an idea of how long it would be? --------------------------------------------------------------------------- Brendan Jurd wrote: > On 11/1/07, Bruce Momjian <bruce@momjian.us> wrote: > > > > I have not forgotten this suggestion. Do have any ideas what such a > > list would look like? Examples? > > > > Thanks for the reply Bruce. > > Code examples, perhaps with "good style" and "bad style" versions to > illustrate each point. > > In the case of Tom's OP about making all your GETARG calls at the top > of the function, you could show (for a trivial function) how it looks > with all the GETARGs stacked neatly at the top, and how it looks with > the GETARGs scattered and inaccurate. > > I think that would make it immediately clear to any newbie why it's a good idea. > > > I think we have avoided more details in fear of scaring off coders. > > People usually follow our style as they gain experience. Having a hard > > list seems like it would be a lot of do's and don't's. > > Just my perspective, but I think you'll scare off a lot more coders by > giving them no firm guidance in the first place, and then jumping down > their throats with "you did this wrong" when they post a patch. > > Might be worth opening up a wiki page for devs to contribute their > insights about writing excellent (as opposed to merely passable) > Postgres code. The GETARG rant could make a good starting point. > > Cheers > BJ > > ---------------------------(end of broadcast)--------------------------- > TIP 2: Don't 'kill -9' the postmaster -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On 11/6/07, Bruce Momjian <bruce@momjian.us> wrote: > > I understand your suggestions but it seems there would be too many > individual items to be readable. Can you suggest a full list so we can > get an idea of how long it would be? If the body of material on writing good Postgres code becomes so comprehensive that it doesn't all fit on one page: a) I would see that as a positive!b) We can of course split it up into sub-articles. I can't realistically suggest a "full list" since I am not an experienced Postgres hacker. But I would hope for, as a minimum: * the stuff about good GETARG style,* something about using ereport() effectively, and writing localization-friendly messages,* some actual coding style guidelines. Regards, BJ
Brendan Jurd wrote: > On 11/6/07, Bruce Momjian <bruce@momjian.us> wrote: > > > > I understand your suggestions but it seems there would be too many > > individual items to be readable. Can you suggest a full list so we can > > get an idea of how long it would be? > > If the body of material on writing good Postgres code becomes so > comprehensive that it doesn't all fit on one page: > > a) I would see that as a positive! > b) We can of course split it up into sub-articles. > > I can't realistically suggest a "full list" since I am not an > experienced Postgres hacker. But I would hope for, as a minimum: > > * the stuff about good GETARG style, > * something about using ereport() effectively, and writing > localization-friendly messages, > * some actual coding style guidelines. The problem is that a full list would be harder to understand than just looking at the existing code and following it, or taking suggestions from us as we review the patch. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On 11/8/07, Bruce Momjian <bruce@momjian.us> wrote: > The problem is that a full list would be harder to understand than just > looking at the existing code and following it, or taking suggestions > from us as we review the patch. > What makes you say it would be necessarily harder to understand? That seems strange to me. Isn't that a bit like saying that the Postgres developers' FAQ is harder to understand than just asking the question on -hackers? What I'm talking about isn't weird. Large collaborative projects often have style guides. To take an example, Python has one for C code [1] and another for Python code [2]. They are clear, useful and easy to understand. [1] http://www.python.org/dev/peps/pep-0007/ [2] http://www.python.org/dev/peps/pep-0008/ Regards, BJ
"Brendan Jurd" <direvus@gmail.com> writes: > They are clear, useful and easy to understand. > > [1] http://www.python.org/dev/peps/pep-0007/ > [2] http://www.python.org/dev/peps/pep-0008/ I didn't look at the second but the first at least is a good example of a style guide which is *not* useful. It's dominated by discussions of white-space and other formatting issues. The only points in this style guide which seem at all useful is the bits near the beginning about not using GCC extensions which hardly needs stating... to the extent that it's even true. None of these points in here seem at all analogous to the important kind of style details like what Tom was pointing out about using GETARG_* at the top of your function to make the argument types clear. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Get trained by Bruce Momjian - ask me about EnterpriseDB'sPostgreSQL training!
Gregory Stark wrote: > > "Brendan Jurd" <direvus@gmail.com> writes: > > > They are clear, useful and easy to understand. > > > > [1] http://www.python.org/dev/peps/pep-0007/ > > [2] http://www.python.org/dev/peps/pep-0008/ > > I didn't look at the second but the first at least is a good example of a > style guide which is *not* useful. It's dominated by discussions of > white-space and other formatting issues. Actually it is excellent guide because we use most of the same rules. (PEP 8 is about code style for code written in Python) -- Alvaro Herrera http://www.advogato.org/person/alvherre "I'm always right, but sometimes I'm more right than other times." (LinusTorvalds)
On Nov 8, 2007 2:49 AM, Gregory Stark <stark@enterprisedb.com> wrote: > > None of these points in here seem at all analogous to the important kind of > style details like what Tom was pointing out about using GETARG_* at the top > of your function to make the argument types clear. > I would love to see a Postgres style guide which includes both: things that are peculiar to the Postgres codebase like GETARG, and an actual standard on how to use whitespace, formatting and naming consistently. If Postgres did have something akin to the Python C style guide, that would be excellent. But all we've got is a standard tabstop of four spaces and the five words "Our standard format BSD style". Don't you think that comes across as pretty weak for a project of this size and significance? Cheers, BJ
"Brendan Jurd" <direvus@gmail.com> writes: > If Postgres did have something akin to the Python C style guide, that > would be excellent. But all we've got is a standard tabstop of four > spaces and the five words "Our standard format BSD style". Don't you > think that comes across as pretty weak for a project of this size and > significance? So you're saying we should adopt a code standard because it looks good? White space and formatting issues just aren't that big a deal. It's professional courtesy to submit a patch that follows the style of the surrounding code, but we don't even insist on that for patches. We reformat with pgindent periodically anyways. It's like a doctor's practice concerned about malpractice claims publishing a written guideline explicitly listing which articles of clothing aren't professional looking enough. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's On-Demand Production Tuning
Gregory Stark wrote: > "Brendan Jurd" <direvus@gmail.com> writes: > > > If Postgres did have something akin to the Python C style guide, that > > would be excellent. But all we've got is a standard tabstop of four > > spaces and the five words "Our standard format BSD style". Don't you > > think that comes across as pretty weak for a project of this size and > > significance? > > So you're saying we should adopt a code standard because it looks good? We already _have_ a code standard. We're just not making it explicit. -- Alvaro Herrera Developer, http://www.PostgreSQL.org/ "Los dioses no protegen a los insensatos. Ãstos reciben protección de otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
"Brendan Jurd" <direvus@gmail.com> writes:
> If Postgres did have something akin to the Python C style guide, that
> would be excellent. But all we've got is a standard tabstop of four
> spaces and the five words "Our standard format BSD style". Don't you
> think that comes across as pretty weak for a project of this size and
> significance?
The reason it's not necessary to be very explicit about that is simple:
pg_indent. Your code *will* conform to the layout standards by the
time it's released ;-). Now it's still a good idea to make new code
look roughly like what you see there already, because if you go too
far overboard in ignoring line-length or comment layout conventions,
the code may look a bit awful when pg_indent gets done with it.
But I see no need to burden authors with any advice more detailed
than "make it look like what you see".
Having said that, there are two or three tips worth knowing about
pg_indent's behavior, like when and how to use dashes to prevent
comment blocks from being re-flowed. But it's a short list.
regards, tom lane
Tom Lane wrote: > "Brendan Jurd" <direvus@gmail.com> writes: > > If Postgres did have something akin to the Python C style guide, that > > would be excellent. But all we've got is a standard tabstop of four > > spaces and the five words "Our standard format BSD style". Don't you > > think that comes across as pretty weak for a project of this size and > > significance? > > The reason it's not necessary to be very explicit about that is simple: > pg_indent. Your code *will* conform to the layout standards by the > time it's released ;-). Now it's still a good idea to make new code > look roughly like what you see there already, because if you go too > far overboard in ignoring line-length or comment layout conventions, > the code may look a bit awful when pg_indent gets done with it. > But I see no need to burden authors with any advice more detailed > than "make it look like what you see". > > Having said that, there are two or three tips worth knowing about > pg_indent's behavior, like when and how to use dashes to prevent > comment blocks from being re-flowed. But it's a short list. Agreed, and the developer's FAQ already has this: <P><I>pgindent</I> will the format code by specifying flags to your operating system's utility <I>indent.</I> This <Ahref= "http://ezine.daemonnews.org/200112/single_coding_style.html">article</A> describes the value of a consistentcoding style.</P> <P><I>pgindent</I> is run on all source files just before each beta test period. It auto-formats all source files tomake them consistent. Comment blocks that need specific line breaks should be formatted as <I>block comments,</I> wherethe comment starts as <CODE>/*------</CODE>. These comments will not be reformatted in any way.</P> -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
"Bruce Momjian" <bruce@momjian.us> writes: > Tom Lane wrote: > >> Having said that, there are two or three tips worth knowing about >> pg_indent's behavior, like when and how to use dashes to prevent >> comment blocks from being re-flowed. But it's a short list. > > Agreed, and the developer's FAQ already has this: > > <P><I>pgindent</I> will the format code by specifying flags to your What I would like to see are guidelines on the more important issues. Things like always using PG_GETARG_* and DatumGet* rather than explicitly calling pg_detoast_datum() and so on. Actually I'm not sure what else would fall under "and so on". -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support!
Bruce Momjian wrote: > Tom Lane wrote: > > "Brendan Jurd" <direvus@gmail.com> writes: > > > If Postgres did have something akin to the Python C style guide, that > > > would be excellent. But all we've got is a standard tabstop of four > > > spaces and the five words "Our standard format BSD style". Don't you > > > think that comes across as pretty weak for a project of this size and > > > significance? > > > > The reason it's not necessary to be very explicit about that is simple: > > pg_indent. Your code *will* conform to the layout standards by the > > time it's released ;-). Now it's still a good idea to make new code > > look roughly like what you see there already, because if you go too > > far overboard in ignoring line-length or comment layout conventions, > > the code may look a bit awful when pg_indent gets done with it. > > But I see no need to burden authors with any advice more detailed > > than "make it look like what you see". > > > > Having said that, there are two or three tips worth knowing about > > pg_indent's behavior, like when and how to use dashes to prevent > > comment blocks from being re-flowed. But it's a short list. If someone submits a piece of code that's totally out of line with our standards, we will ask him to resubmit. This step could be avoided if he knew what those standards were in the first place. This doesn't have to be a bible on coding style. A few guidelines suffice. PG_GETARG and the like are not fixed by pgindent though (which is what spawned this whole discussion). And in-house code (or pgFondry code), which could also benefit by our coding style, will not be processed by pgindent anyway. > <P><I>pgindent</I> will the format code by specifying flags to your > operating system's utility <I>indent.</I> This <A href= > "http://ezine.daemonnews.org/200112/single_coding_style.html">article</A> > describes the value of a consistent coding style.</P> I don't understand why would you link to an article about the value of consistent coding style, and not provide a link to our own coding style. I also don't understand why pgindent is assumed to be some sort of silver bullet to solve all our coding style problems. It is better if we educate our developers instead of just having pgindent at the end of the cycle deal with the messed up code. Up to now, this education has been one individual at a time, but it is much better if we can scale in a way that every individual wastes only his own time learning our rules. -- Alvaro Herrera Valdivia, Chile ICBM: S 39º 49' 18.1", W 73º 13' 56.4" Thou shalt check the array bounds of all strings (indeed, all arrays), for surely where thou typest "foo" someone someday shall type "supercalifragilisticexpialidocious" (5th Commandment for C programmers)
Alvaro Herrera wrote: > > > Having said that, there are two or three tips worth knowing about > > > pg_indent's behavior, like when and how to use dashes to prevent > > > comment blocks from being re-flowed. But it's a short list. > > If someone submits a piece of code that's totally out of line with our > standards, we will ask him to resubmit. This step could be avoided if > he knew what those standards were in the first place. True, but 95% of the cases are submitters using the wrong functions, methods, etc, not the style. I don't remember us ever returning a patch for rework based on style issues alone. > PG_GETARG and the like are not fixed by pgindent though (which is what > spawned this whole discussion). And in-house code (or pgFondry code), > which could also benefit by our coding style, will not be processed by > pgindent anyway. > > > <P><I>pgindent</I> will the format code by specifying flags to your > > operating system's utility <I>indent.</I> This <A href= > > "http://ezine.daemonnews.org/200112/single_coding_style.html">article</A> > > describes the value of a consistent coding style.</P> > > I don't understand why would you link to an article about the value of > consistent coding style, and not provide a link to our own coding style. > > I also don't understand why pgindent is assumed to be some sort of > silver bullet to solve all our coding style problems. It is better if > we educate our developers instead of just having pgindent at the end of > the cycle deal with the messed up code. Up to now, this education has > been one individual at a time, but it is much better if we can scale in > a way that every individual wastes only his own time learning our rules. We want patch submitters to spend their time on patches, not learning our style. The fact is that pgindent is a silver bullet in some ways. My major contention is that any list is going to be very details and hard to read, and no one has put up a list disputing that. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Alvaro Herrera <alvherre@commandprompt.com> writes:
> If someone submits a piece of code that's totally out of line with our
> standards, we will ask him to resubmit. This step could be avoided if
> he knew what those standards were in the first place.
True, but "make it look like what you see" is more than enough detail
for that purpose. We should not expend a lot of thought-bandwidth
on details that pgindent can fix.
> I also don't understand why pgindent is assumed to be some sort of
> silver bullet to solve all our coding style problems.
It certainly cannot fix high-level style issues, but issues like brace
placement and indenting it can deal with; and that seems to be what
too many people are talking about when they mention coding style.
regards, tom lane
On Nov 9, 2007 3:17 AM, Bruce Momjian <bruce@momjian.us> wrote: > We want patch submitters to spend their time on patches, not learning > our style. The fact is that pgindent is a silver bullet in some ways. Well there's a lot of support for the idea of pgindent being good enough to establish a consistent coding style. I personally think a much higher target for consistency would be worth pursuing, but I can tell when I'm outgunned. Maybe it would be more productive to drop the topic of code style (as in whitespace/formatting) and stick to talking about code style (as in GETARG). I've suggested that some more information on using ereport effectively might be at home in such a list. Perhaps some advice about working with varlenas (which macros you should use in given situations, differentiating toasted and detoasted). Are there any items which patch reviewers find themselves repeating to several different developers? Things that follow the form "Don't use $foo, use $bar", or "We don't do $x anymore, do $y instead"? These are the sorts of items that would really benefit from publication. > > My major contention is that any list is going to be very details and > hard to read, and no one has put up a list disputing that. > This complaint still puzzles me. Why would it be hard to read? Wouldn't that have more to do with the way it is presented than what it contains? If it turns out to be hard to read, that's just an indication that it needs to be better formatted. This isn't superstring theory. It's just some guidelines on how to write good Postgres code. Even if it were hard to read, reading a difficult document is pretty much guaranteed to take less time than waiting on a full review cycle, then reworking, recompiling, retesting and resubmitting your patch. Cheers BJ
Brendan Jurd wrote: > On Nov 9, 2007 3:17 AM, Bruce Momjian <bruce@momjian.us> wrote: > > We want patch submitters to spend their time on patches, not learning > > our style. The fact is that pgindent is a silver bullet in some ways. > > Well there's a lot of support for the idea of pgindent being good > enough to establish a consistent coding style. I personally think a > much higher target for consistency would be worth pursuing, but I can > tell when I'm outgunned. > > Maybe it would be more productive to drop the topic of code style (as > in whitespace/formatting) and stick to talking about code style (as in > GETARG). > > I've suggested that some more information on using ereport effectively > might be at home in such a list. Perhaps some advice about working > with varlenas (which macros you should use in given situations, > differentiating toasted and detoasted). > > Are there any items which patch reviewers find themselves repeating to > several different developers? Things that follow the form "Don't use > $foo, use $bar", or "We don't do $x anymore, do $y instead"? These > are the sorts of items that would really benefit from publication. I can't think of anything that isn't already in the developer's FAQ. > > My major contention is that any list is going to be very details and > > hard to read, and no one has put up a list disputing that. > > > > This complaint still puzzles me. Why would it be hard to read? > Wouldn't that have more to do with the way it is presented than what > it contains? If it turns out to be hard to read, that's just an > indication that it needs to be better formatted. This isn't > superstring theory. It's just some guidelines on how to write good > Postgres code. It is going to be lots of minutia that is going to be very unintersting and saying just follow the surrounding code is a better use of their time rather than reading a list. > Even if it were hard to read, reading a difficult document is pretty > much guaranteed to take less time than waiting on a full review cycle, > then reworking, recompiling, retesting and resubmitting your patch. We just don't see that happening much in practice. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +