gratuitous casting away const

Mark Dilger <hornschnorter@gmail.com> writes:

Would patches to not cast away const be considered?

In general, yes, but I'm not especially in favor of something like this:

bool
PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
-                                               Item newtup, Size newsize)
+                                               const char *newtup, Size newsize)
{

since that seems to be discarding type information in order to add
"const"; does not seem like a net benefit from here.

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

On Sep 20, 2016, at 1:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Mark Dilger <hornschnorter@gmail.com> writes:

Would patches to not cast away const be considered?

In general, yes, but I'm not especially in favor of something like this:

bool
PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
-                                               Item newtup, Size newsize)
+                                               const char *newtup, Size newsize)
{

since that seems to be discarding type information in order to add
"const"; does not seem like a net benefit from here.

The following seems somewhere in between, with ItemPointer
changing to const ItemPointerData *. I expect you would not care
for this change, but thought I'd check to see where you draw the line:

diff --git a/src/backend/access/gin/ginbulk.c b/src/backend/access/gin/ginbulk.c
index 71c64e4..903b01f 100644
--- a/src/backend/access/gin/ginbulk.c
+++ b/src/backend/access/gin/ginbulk.c
@@ -244,7 +244,7 @@ ginInsertBAEntries(BuildAccumulator *accum,
 static int
 qsortCompareItemPointers(const void *a, const void *b)
 {
-   int         res = ginCompareItemPointers((ItemPointer) a, (ItemPointer) b);
+   int         res = ginCompareItemPointers((const ItemPointerData *) a, (const ItemPointerData *) b);

    /* Assert that there are no equal item pointers being sorted */
    Assert(res != 0);
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index bf589ab..2e5a7dff 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -968,7 +968,7 @@ extern ItemPointer ginMergeItemPointers(ItemPointerData *a, uint32 na,
  * so we want this to be inlined.
  */
 static inline int
-ginCompareItemPointers(ItemPointer a, ItemPointer b)
+ginCompareItemPointers(const ItemPointerData *a, const ItemPointerData *b)
 {
    uint64      ia = (uint64) a->ip_blkid.bi_hi << 32 | (uint64) a->ip_blkid.bi_lo << 16 | a->ip_posid;
    uint64      ib = (uint64) b->ip_blkid.bi_hi << 32 | (uint64) b->ip_blkid.bi_lo << 16 | b->ip_posid;

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Mark Dilger <hornschnorter@gmail.com> writes:

On Sep 20, 2016, at 1:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
... that seems to be discarding type information in order to add
"const"; does not seem like a net benefit from here.

The following seems somewhere in between, with ItemPointer
changing to const ItemPointerData *. I expect you would not care
for this change, but thought I'd check to see where you draw the line:

I'd call this kind of a wash, I guess. I'd be more excited about it if
the change allowed removal of an actual cast-away-of-constness somewhere.

I suppose it's a bit of a chicken and egg situation, in that the lack
of const markings on leaf subroutines discourages use of "const" in
callers, and you have to start somewhere if you want to make it better.
But I don't really want to just plaster "const" onto individual functions
without some larger vision of where we're going and which code is going
to benefit. Otherwise it seems like mostly just code churn.

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

On Sep 22, 2016, at 9:14 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd call this kind of a wash, I guess. I'd be more excited about it if
the change allowed removal of an actual cast-away-of-constness somewhere.

I suppose it's a bit of a chicken and egg situation, in that the lack
of const markings on leaf subroutines discourages use of "const" in
callers, and you have to start somewhere if you want to make it better.
But I don't really want to just plaster "const" onto individual functions
without some larger vision of where we're going and which code is going
to benefit. Otherwise it seems like mostly just code churn.

regards, tom lane

I have two purposes in doing this. First, I find the code more self-documenting
this way. Second, I can get whole directories to compile cleanly without
warnings using the -Wcast-qual flag, where currently that flag results in
warnings. That makes it possible to add cast-qual to more individual source
directories' Makefiles than I can currently do while still using -Werror in
Makefile.global.

Now, I'm not proposing that everybody else needs to have -Wcast-qual. I'm
just saying that I'd like to be able to have that in my copy of the project.

mark

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Friends,

here is another patch, this time fixing the casting away of const
in the regex code.

Mark Dilger