Horton

Thread

Commits

  1. Add const qualifiers to XLogRegister*() functions

  2. Constify crc32_sz

  3. Modernize const handling with readline

  1. Modernize const handling with readline

    Peter Eisentraut <peter@eisentraut.org> — 2023-10-03T10:23:12Z

    The comment

     /* On some platforms, readline is declared as readline(char *) */

is obsolete.  The casting away of const can be removed.

The const in the readline() prototype was added in GNU readline 4.2, 
released in 2001.  BSD libedit has also had const in the prototype since 
at least 2001.

(The commit that introduced this comment (187e865174) talked about 
FreeBSD 4.8, which didn't have readline compatibility in libedit yet, so 
it must have been talking about GNU readline in the base system.  This 
checks out, but already FreeBSD 5 had an updated GNU readline with const.)

  2. Re: Modernize const handling with readline

    Aleksander Alekseev <aleksander@timescale.com> — 2023-10-03T11:28:18Z

    Hi,

> The comment
>
>      /* On some platforms, readline is declared as readline(char *) */
>
> is obsolete.  The casting away of const can be removed.
>
> The const in the readline() prototype was added in GNU readline 4.2,
> released in 2001.  BSD libedit has also had const in the prototype since
> at least 2001.
>
> (The commit that introduced this comment (187e865174) talked about
> FreeBSD 4.8, which didn't have readline compatibility in libedit yet, so
> it must have been talking about GNU readline in the base system.  This
> checks out, but already FreeBSD 5 had an updated GNU readline with const.)

LGTM.

While examining the code for similar places I noticed that the
following functions can also be const'ified:

- crc32_sz
- pg_checksum_page (? temporary modifies the page but then restores it)
- XLogRegisterData (?)

The callers of cstring_to_text[_with_len] often cast the argument to
(char *) while in fact it's (const char *). This can be refactored
too.

Additionally there is a slight difference between XLogRegisterBlock()
declaration in xloginsert.h:

```
extern void XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator,
                              ForkNumber forknum, BlockNumber blknum,
char *page,
                              uint8 flags);
```

... and xloginsert.c:

```
void
XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator, ForkNumber forknum,
                  BlockNumber blknum, Page page, uint8 flags)
```

Will there be a value in addressing anything of this?

-- 
Best regards,
Aleksander Alekseev

  3. Re: Modernize const handling with readline

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-03T14:10:38Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> The comment
>      /* On some platforms, readline is declared as readline(char *) */
> is obsolete.  The casting away of const can be removed.

+1, that's surely not of interest on anything we still support.

			regards, tom lane

  4. Re: Modernize const handling with readline

    Peter Eisentraut <peter@eisentraut.org> — 2023-10-04T14:37:16Z

    On 03.10.23 13:28, Aleksander Alekseev wrote:
> While examining the code for similar places I noticed that the
> following functions can also be const'ified:
> 
> - crc32_sz

I suppose this could be changed.

> - pg_checksum_page (? temporary modifies the page but then restores it)

Then it's not really const?

> - XLogRegisterData (?)

I don't think this would work, at least without further work elsewhere, 
because the data is stored in XLogRecData, which has no const handling.

> The callers of cstring_to_text[_with_len] often cast the argument to
> (char *) while in fact it's (const char *). This can be refactored
> too.

These look fine to me.

> Additionally there is a slight difference between XLogRegisterBlock()
> declaration in xloginsert.h:
> 
> ```
> extern void XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator,
>                                ForkNumber forknum, BlockNumber blknum,
> char *page,
>                                uint8 flags);
> ```
> 
> ... and xloginsert.c:
> 
> ```
> void
> XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator, ForkNumber forknum,
>                    BlockNumber blknum, Page page, uint8 flags)
> ```

It looks like the reason here is that xloginsert.h does not have the 
Page type in scope.  I don't know how difficult it would be to change 
that, but it seems fine as is.

  5. Re: Modernize const handling with readline

    Aleksander Alekseev <aleksander@timescale.com> — 2023-10-04T15:09:36Z

    Hi,

> On 03.10.23 13:28, Aleksander Alekseev wrote:
> > While examining the code for similar places I noticed that the
> > following functions can also be const'ified:
> >
> > - crc32_sz
>
> I suppose this could be changed.

OK, that's a simple change. Here is the patch.

-- 
Best regards,
Aleksander Alekseev

  6. Re: Modernize const handling with readline

    Peter Eisentraut <peter@eisentraut.org> — 2023-10-05T06:57:51Z

    On 04.10.23 17:09, Aleksander Alekseev wrote:
> Hi,
> 
>> On 03.10.23 13:28, Aleksander Alekseev wrote:
>>> While examining the code for similar places I noticed that the
>>> following functions can also be const'ified:
>>>
>>> - crc32_sz
>>
>> I suppose this could be changed.
> 
> OK, that's a simple change. Here is the patch.

committed this and my patch

  7. Add const qualifiers to XLogRegister*() functions

    Peter Eisentraut <peter@eisentraut.org> — 2024-08-28T08:09:27Z

    On 04.10.23 16:37, Peter Eisentraut wrote:
> On 03.10.23 13:28, Aleksander Alekseev wrote:
>> While examining the code for similar places I noticed that the
>> following functions can also be const'ified:

>> - XLogRegisterData (?)
> 
> I don't think this would work, at least without further work elsewhere, 
> because the data is stored in XLogRecData, which has no const handling.

I got around to fixing this.  Here is a patch.  It allows removing a few 
unconstify() calls, which is nice.

  8. Re: Add const qualifiers to XLogRegister*() functions

    Aleksander Alekseev <aleksander@timescale.com> — 2024-08-28T10:04:38Z

    Hi,

> On 04.10.23 16:37, Peter Eisentraut wrote:
> > On 03.10.23 13:28, Aleksander Alekseev wrote:
> >> While examining the code for similar places I noticed that the
> >> following functions can also be const'ified:
>
> >> - XLogRegisterData (?)
> >
> > I don't think this would work, at least without further work elsewhere,
> > because the data is stored in XLogRecData, which has no const handling.
>
> I got around to fixing this.  Here is a patch.  It allows removing a few
> unconstify() calls, which is nice.

LGTM.

Note that this may affect third-party code. IMO this is not a big deal
in this particular case.

Also by randomly checking one of the affected non-static functions I
found a bunch of calls like this:

XLogRegisterData((char *) msgs, ...)

... where the first argument is going to become (const char *). It
looks like the compilers are OK with implicitly casting (char*) to a
more restrictive (const char*) though.

--
Best regards,
Aleksander Alekseev

  9. Re: Add const qualifiers to XLogRegister*() functions

    Peter Eisentraut <peter@eisentraut.org> — 2024-09-03T06:15:29Z

    On 28.08.24 12:04, Aleksander Alekseev wrote:
> Hi,
> 
>> On 04.10.23 16:37, Peter Eisentraut wrote:
>>> On 03.10.23 13:28, Aleksander Alekseev wrote:
>>>> While examining the code for similar places I noticed that the
>>>> following functions can also be const'ified:
>>
>>>> - XLogRegisterData (?)
>>>
>>> I don't think this would work, at least without further work elsewhere,
>>> because the data is stored in XLogRecData, which has no const handling.
>>
>> I got around to fixing this.  Here is a patch.  It allows removing a few
>> unconstify() calls, which is nice.
> 
> LGTM.

committed

> Note that this may affect third-party code. IMO this is not a big deal
> in this particular case.

I don't think this will impact any third-party code.  Only maybe for the 
better, by being able to remove some casts.

> Also by randomly checking one of the affected non-static functions I
> found a bunch of calls like this:
> 
> XLogRegisterData((char *) msgs, ...)
> 
> ... where the first argument is going to become (const char *). It
> looks like the compilers are OK with implicitly casting (char*) to a
> more restrictive (const char*) though.

Yes, that's ok.