Re: [HACKERS] PATCH: Spinlock Documentation
On Sat, Apr 11, 2015 at 12:03 PM, Artem Luzyanin lisyono...@yahoo.ca wrote: Thank you again for your feedback. I have improved the patch with your suggestions. Please let me know what you think and if I can do anything else. Current CommitFest link for this patch is: https://commitfest.postgresql.org/5/208/ Some review comments: - The first hunk in s_lock.h touches only whitespace. Changing the space to a tab on the Usually line would make sense for consistency, but adding a trailing space to the override them line does not. - As Tom basically said before, I think the File layout block comment will just get out of date and be a maintenance annoyance to future updaters of this file. It's not really that hard to see the structure of the file just by going through it, so I don't think this is worthwhile. - Similarly, adding all of the Currently implemented lines looks useless to me. Why can't somebody see that from just reading the code itself? Overall, I'm not seeing much point to this patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Spinlock Documentation
Hello, Thank you again for your feedback. I have improved the patch with your suggestions. Please let me know what you think and if I can do anything else. Current CommitFest link for this patch is: https://commitfest.postgresql.org/5/208/ Respectfully, Artem Luzyanin On Sunday, April 5, 2015 5:59 PM, Artem Luzyanin lisyono...@yahoo.ca wrote: Hello, Thank you very much for your feedback! I will work on the changes as soon as I can. Respectfully, Artem Luzyanin On Sunday, April 5, 2015 5:45 PM, Tom Lane t...@sss.pgh.pa.us wrote: David Fetter da...@fetter.org writes: One issue with this patch is that it is not localized. If someone goes and changes the S_LOCK implementation for one of the platforms below, or adds a new platform, etc., without changing this comment too, this comment becomes confusingly obsolete. Indeed. Moreover, this header comment is supposed to be an overview and specification of the macros that need to be provided. I think it's an actively bad idea to clutter it with platform-by-platform details; that will create a can't see the forest for the trees problem. If we need more info here, I think a comment block before each section of the file would make more sense. But the patch as provided seems like it would just be redundant if it were refactored in that form. What would possibly be useful that's not there now is a paragraph or two describing the overall layout of the file (eg gcc then non gcc, or whatever can be said at more or less that level of detail). But please don't stick that into the middle of the specification part. regards, tom lane spinlock-docsV2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Spinlock Documentation
David Fetter da...@fetter.org writes: One issue with this patch is that it is not localized. If someone goes and changes the S_LOCK implementation for one of the platforms below, or adds a new platform, etc., without changing this comment too, this comment becomes confusingly obsolete. Indeed. Moreover, this header comment is supposed to be an overview and specification of the macros that need to be provided. I think it's an actively bad idea to clutter it with platform-by-platform details; that will create a can't see the forest for the trees problem. If we need more info here, I think a comment block before each section of the file would make more sense. But the patch as provided seems like it would just be redundant if it were refactored in that form. What would possibly be useful that's not there now is a paragraph or two describing the overall layout of the file (eg gcc then non gcc, or whatever can be said at more or less that level of detail). But please don't stick that into the middle of the specification part. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Spinlock Documentation
Hello, Thank you very much for your feedback! I will work on the changes as soon as I can. Respectfully, Artem Luzyanin On Sunday, April 5, 2015 5:45 PM, Tom Lane t...@sss.pgh.pa.us wrote: David Fetter da...@fetter.org writes: One issue with this patch is that it is not localized. If someone goes and changes the S_LOCK implementation for one of the platforms below, or adds a new platform, etc., without changing this comment too, this comment becomes confusingly obsolete. Indeed. Moreover, this header comment is supposed to be an overview and specification of the macros that need to be provided. I think it's an actively bad idea to clutter it with platform-by-platform details; that will create a can't see the forest for the trees problem. If we need more info here, I think a comment block before each section of the file would make more sense. But the patch as provided seems like it would just be redundant if it were refactored in that form. What would possibly be useful that's not there now is a paragraph or two describing the overall layout of the file (eg gcc then non gcc, or whatever can be said at more or less that level of detail). But please don't stick that into the middle of the specification part. regards, tom lane
Re: [HACKERS] PATCH: Spinlock Documentation
On Sun, Apr 05, 2015 at 06:50:59PM +, Artem Luzyanin wrote: Hello, I am new to PostgreSQLcommunity, but I would like to become a contributer eventually. I have readthrough your Submitting Patch guide and decided to follow Start with submitting a patch that is small anduncontroversial to help them understand you, and to get you familiar with theoverall process suggestion. I am interested inplatform-specific spinlock implementation, so I looked at s_lock.h file for possibleimprovement. Since it took me some time to find possible areas of improvement,I would like to submit a small patch that would facilitate the process forfuture contributors (including myself). Since this is my first e-mail, pleaselet me know if I should have done something differently in order to submit apatch for the community. One issue with this patch is that it is not localized. If someone goes and changes the S_LOCK implementation for one of the platforms below, or adds a new platform, etc., without changing this comment too, this comment becomes confusingly obsolete. How do you plan to address this issue? Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
