Thread: gratuitous casting away const
Friends,
There are places in the code that cast away const for no apparent
reason, fixed like such:
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index 3143bd9..fe2cfc2 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -409,8 +409,8 @@ static intreorderqueue_cmp(const pairingheap_node *a, const pairingheap_node *b,
void *arg){
- ReorderTuple *rta = (ReorderTuple *) a;
- ReorderTuple *rtb = (ReorderTuple *) b;
+ const ReorderTuple *rta = (const ReorderTuple *) a;
+ const ReorderTuple *rtb = (const ReorderTuple *) b; IndexScanState *node = (IndexScanState *) arg;
return-cmp_orderbyvals(rta->orderbyvals, rta->orderbynulls,
There are also places which appear to cast away const due to using
typedefs that don't include const, which make for a more messy fix,
like such:
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index 73aa0c0..9e157a3 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -1037,7 +1037,7 @@ PageIndexTupleDeleteNoCompact(Page page, OffsetNumber offnum) */boolPageIndexTupleOverwrite(Page
page,OffsetNumber offnum,
- Item newtup, Size newsize)
+ const char *newtup, Size newsize){ PageHeader phdr =
(PageHeader)page; ItemId tupid;
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index ad4ab5f..cd97a1a 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -431,7 +431,7 @@ extern void PageIndexTupleDelete(Page page, OffsetNumber offset);extern void
PageIndexMultiDelete(Pagepage, OffsetNumber *itemnos, int nitems);extern void PageIndexTupleDeleteNoCompact(Page page,
OffsetNumberoffset);extern bool PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
- Item newtup, Size newsize);
+ const char *newtup, Size newsize);extern char *PageSetChecksumCopy(Page
page,BlockNumber blkno);extern void PageSetChecksumInplace(Page page, BlockNumber blkno);
Are these castings away of const consistent with the project's coding standards?
Would patches to not cast away const be considered?
Thanks,
Mark Dilger
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
> 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 intqsortCompareItemPointers(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;
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
> 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
Friends, here is another patch, this time fixing the casting away of const in the regex code. Mark Dilger