Re: [HACKERS] libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved
On tis, 2012-01-10 at 14:10 -0500, Alex Goncharov wrote: | Note that const PGresult * would only warn against changing the | fields It would not warn, it would err (the compilation should fail). No, const violations generally only produce warnings. | of the PGresult struct. It doesn't do anything about changing the data | pointed to by pointers in the PGresult struct. So what you are saying | doesn't follow. By this logic, passing 'const struct foo *' doesn't have any point and value, for any function. It pretty much doesn't. But we know that this is done (and thank you for that) in many cases -- a good style, self-documentation and some protection. I'm not disagreeing, but I'm pointing out that it won't do what you expect. -- 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] libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved
On tis, 2011-12-13 at 07:55 -0500, Alex Goncharov wrote: char *PQcmdStatus(PGresult *res); char *PQcmdTuples(PGresult *res); Unreasonable: a. What, these two can modify 'res' I pass in?.. b. Oh, yes, because they return 'char *' pointing to 'res-cmdStatus+n', so, a libpq user may write: char *s = PQcmdStatus(res); *s = 'x'; and have 'res' modified. (Would be the user's fault, of course.) Note that const PGresult * would only warn against changing the fields of the PGresult struct. It doesn't do anything about changing the data pointed to by pointers in the PGresult struct. So what you are saying doesn't follow. -- 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] libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved
,--- You/Peter (Tue, 10 Jan 2012 19:13:42 +0200) * | On tis, 2011-12-13 at 07:55 -0500, Alex Goncharov wrote: |char *PQcmdStatus(PGresult *res); |char *PQcmdTuples(PGresult *res); | | Unreasonable: | |a. What, these two can modify 'res' I pass in?.. | |b. Oh, yes, because they return 'char *' pointing to | 'res-cmdStatus+n', so, a libpq user may write: | | char *s = PQcmdStatus(res); | *s = 'x'; | | and have 'res' modified. (Would be the user's fault, of course.) | | Note that const PGresult * would only warn against changing the | fields It would not warn, it would err (the compilation should fail). | of the PGresult struct. It doesn't do anything about changing the data | pointed to by pointers in the PGresult struct. So what you are saying | doesn't follow. By this logic, passing 'const struct foo *' doesn't have any point and value, for any function. But we know that this is done (and thank you for that) in many cases -- a good style, self-documentation and some protection. E.g. here: ,--- I/Alex (Tue, 13 Dec 2011 07:55:45 -0500) * | Compare: | | int PQntuples(const PGresult *res) | | Reasonable: doesn't modify 'res'. `-* BTW, I have not submitted the context differences, as suggested, only because of extreme overload at work and the need to do a careful caller and documentation analysis. I still hope to be able to do it in a reasonably near future. -- Alex -- alex-goncha...@comcast.net -- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved
Compare: int PQntuples(const PGresult *res) Reasonable: doesn't modify 'res'. With: char *PQcmdStatus(PGresult *res); char *PQcmdTuples(PGresult *res); Unreasonable: a. What, these two can modify 'res' I pass in?.. b. Oh, yes, because they return 'char *' pointing to 'res-cmdStatus+n', so, a libpq user may write: char *s = PQcmdStatus(res); *s = 'x'; and have 'res' modified. (Would be the user's fault, of course.) The non-const-ness of 'PGresult *' for these two functions seems to stand out among the functions covered in the 30.3.2. Retrieving Query Result Information manual section and inhibits writing the strict client code. I would suggest to change the signatures by applying this trivial patch (and changing the documentation): == diff orig/postgresql-9.1.1/src/interfaces/libpq/libpq-fe.h ./postgresql-9.1.1/src/interfaces/libpq/libpq-fe.h 450c450 extern char *PQcmdStatus(PGresult *res); --- extern const char *PQcmdStatus(const PGresult *res); 453c453 extern char *PQcmdTuples(PGresult *res); --- extern const char *PQcmdTuples(const PGresult *res); == diff orig/postgresql-9.1.1/src/interfaces/libpq/fe-exec.c ./postgresql-9.1.1/src/interfaces/libpq/fe-exec.c 2665,2666c2665,2666 char * PQcmdStatus(PGresult *res) --- const char * PQcmdStatus(const PGresult *res) 2736,2737c2736,2737 char * PQcmdTuples(PGresult *res) --- const char * PQcmdTuples(const PGresult *res) 2739,2740c2739 char *p, *c; --- const char *p, *c; (The above was obtained in 9.1.1; the subsequent build with GCC 4.1.2 succeeds without warnings.) If the above change causes a warning in a client code, so much the better: the client code is doing something unreasonable like the *s assignment in my example above. -- Alex -- alex-goncha...@comcast.net -- -- 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] libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved
On Tue, Dec 13, 2011 at 7:55 AM, Alex Goncharov alex-goncha...@comcast.net wrote: If the above change causes a warning in a client code, so much the better: the client code is doing something unreasonable like the *s assignment in my example above. Or they just haven't bothered to decorate their entire code-base with const declarations. I suppose it's probably worth doing this, but I reserve the right to be unexcited about it. At a minimum, I dispute the application of the term painless to any change involving const. If you want this patch to be considered for application, you should post an updated patch which includes the necessary doc changes and add a link to it here: https://commitfest.postgresql.org/action/commitfest_view/open -- 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] libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved
,--- I/Alex (Tue, 13 Dec 2011 07:55:45 -0500) * | If the above change causes a warning in a client code, so much the | better: the client code is doing something unreasonable like the *s | assignment in my example above. ,--- Robert Haas (Tue, 13 Dec 2011 10:51:54 -0500) * | Or they just haven't bothered to decorate their entire code-base with | const declarations. They don't have to, for the conceptually correct code. I.e. one can write (with the old and new code): /* no: const */ PGresult *res; const char *readout; readout = PQxxx(res,...); /* no: *readout = 'x'; */ all right and have no compilation warnings. But one can also (reasonably) const-qualify the 'res' above (const-correct and const-consistent code is a good thing.) | If you want this patch to be considered for application, you should | post an updated patch which includes the necessary doc changes and add | a link to it here: | | https://commitfest.postgresql.org/action/commitfest_view/open OK, I could do it... ,--- Alvaro Herrera (Tue, 13 Dec 2011 13:01:13 -0300) * | Do we really need a 100% complete patch just to discuss whether we're | open to doing it? IMHO it makes sense to see a WIP patch and then | accept or reject based on that; if we accept the general idea, then a | complete patch would presumably be submitted. `-* ... but I like this more. I.e., can one tell me to bother or not with the complete patch, based on the general idea, which wouldn't change for the complete patch? -- Alex -- alex-goncha...@comcast.net -- -- 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] libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved
On Tue, Dec 13, 2011 at 11:01 AM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mar dic 13 12:51:54 -0300 2011: If you want this patch to be considered for application, you should post an updated patch which includes the necessary doc changes and add a link to it here: https://commitfest.postgresql.org/action/commitfest_view/open Do we really need a 100% complete patch just to discuss whether we're open to doing it? Of course not. You may notice that I also offered an opinion on the substance of the patch. In the course of doing that, I don't see why I shouldn't point out that it's the patch author's responsibility to provide docs. YMMV, of course. -- 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] libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved
Excerpts from Alex Goncharov's message of mar dic 13 13:43:35 -0300 2011: I.e., can one tell me to bother or not with the complete patch, based on the general idea, which wouldn't change for the complete patch? So let's see the patch. In context diff format please, and also include in-tree changes to the callers of those functions, if any are necessary. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved
Excerpts from Robert Haas's message of mar dic 13 12:51:54 -0300 2011: If you want this patch to be considered for application, you should post an updated patch which includes the necessary doc changes and add a link to it here: https://commitfest.postgresql.org/action/commitfest_view/open Do we really need a 100% complete patch just to discuss whether we're open to doing it? IMHO it makes sense to see a WIP patch and then accept or reject based on that; if we accept the general idea, then a complete patch would presumably be submitted. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
