libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved

On Tue, Dec 13, 2011 at 7:55 AM, Alex Goncharov
<alex-goncharov@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

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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

,--- 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-goncharov@comcast.net --

On Tue, Dec 13, 2011 at 11:01 AM, Alvaro Herrera
<alvherre@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

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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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.

,--- 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-goncharov@comcast.net --

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.