Horton

Thread

Commits

  1. Fix ILIST_DEBUG build

  2. Constify the arguments of ilist.c/h functions

  1. [PATCH] Const'ify the arguments of ilist.c/ilist.h functions

    Aleksander Alekseev <aleksander@timescale.com> — 2022-11-07T09:03:23Z

    Hi hackers,

During the [1] discussion it was suggested to constify the arguments
of ilist.c/ilist.h functions. Bharath (cc'ed) pointed out that it's
better to start a new thread in order to attract more hackers that may
be interested in this change, so I started one.

The patch is attached. Here are the reasons why we may want to do this:

"""
Const qualifiers ensure that we don't do something stupid in the function
implementation. Additionally they clarify the interface. As an example:

void
slist_delete(slist_head *head, const slist_node *node)

Here one can instantly tell that node->next is not going to be set to NULL.
Finally, const qualifiers potentially allow the compiler to do more
optimizations. This being said no benchmarking was done for this patch.
"""

Additionally Bharath pointed out that there are other pieces of code
that we may want to change in a similar fashion,
proclist.h/proclist_types.h as one example. I didn't do this yet
because I would like to know the community opinion first on whether we
should do this at all.

Thoughts?

[1]: https://www.postgresql.org/message-id/flat/CAApHDvrtVxr+FXEX0VbViCFKDGxA3tWDgw9oFewNXCJMmwLjLg@mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

  2. Re: [PATCH] Const'ify the arguments of ilist.c/ilist.h functions

    Andres Freund <andres@anarazel.de> — 2022-11-22T17:31:56Z

    Hi,

On 2022-11-07 12:03:23 +0300, Aleksander Alekseev wrote:
> During the [1] discussion it was suggested to constify the arguments
> of ilist.c/ilist.h functions. Bharath (cc'ed) pointed out that it's
> better to start a new thread in order to attract more hackers that may
> be interested in this change, so I started one.

I needed something similar in https://postgr.es/m/20221120055930.t6kl3tyivzhlrzu2%40awork3.anarazel.de



> @@ -484,7 +484,7 @@ dlist_has_prev(dlist_head *head, dlist_node *node)
>   * Return the next node in the list (there must be one).
>   */
>  static inline dlist_node *
> -dlist_next_node(dlist_head *head, dlist_node *node)
> +dlist_next_node(const dlist_head *head, const dlist_node *node)
>  {
>  	Assert(dlist_has_next(head, node));
>  	return node->next;
> @@ -494,7 +494,7 @@ dlist_next_node(dlist_head *head, dlist_node *node)
>   * Return previous node in the list (there must be one).
>   */
>  static inline dlist_node *
> -dlist_prev_node(dlist_head *head, dlist_node *node)
> +dlist_prev_node(const dlist_head *head, const dlist_node *node)
>  {
>  	Assert(dlist_has_prev(head, node));
>  	return node->prev;
> @@ -502,7 +502,7 @@ dlist_prev_node(dlist_head *head, dlist_node *node)
>  
>  /* internal support function to get address of head element's struct */
>  static inline void *
> -dlist_head_element_off(dlist_head *head, size_t off)
> +dlist_head_element_off(const dlist_head *head, size_t off)
>  {
>  	Assert(!dlist_is_empty(head));
>  	return (char *) head->head.next - off;
> @@ -512,14 +512,14 @@ dlist_head_element_off(dlist_head *head, size_t off)
>   * Return the first node in the list (there must be one).
>   */
>  static inline dlist_node *
> -dlist_head_node(dlist_head *head)
> +dlist_head_node(const dlist_head *head)
>  {
>  	return (dlist_node *) dlist_head_element_off(head, 0);
>  }
>  
>  /* internal support function to get address of tail element's struct */
>  static inline void *
> -dlist_tail_element_off(dlist_head *head, size_t off)
> +dlist_tail_element_off(const dlist_head *head, size_t off)
>  {
>  	Assert(!dlist_is_empty(head));
>  	return (char *) head->head.prev - off;
> @@ -529,7 +529,7 @@ dlist_tail_element_off(dlist_head *head, size_t off)
>   * Return the last node in the list (there must be one).
>   */
>  static inline dlist_node *
> -dlist_tail_node(dlist_head *head)
> +dlist_tail_node(const dlist_head *head)
>  {
>  	return (dlist_node *) dlist_tail_element_off(head, 0);
>  }

I don't think it is correct for any of these to add const. The only reason it
works is because of casting etc.


> @@ -801,7 +801,7 @@ dclist_has_prev(dclist_head *head, dlist_node *node)
>   *		Return the next node in the list (there must be one).
>   */
>  static inline dlist_node *
> -dclist_next_node(dclist_head *head, dlist_node *node)
> +dclist_next_node(const dclist_head *head, const dlist_node *node)
>  {
>  	Assert(head->count > 0);
>  
> @@ -813,7 +813,7 @@ dclist_next_node(dclist_head *head, dlist_node *node)
>   *		Return the prev node in the list (there must be one).
>   */
>  static inline dlist_node *
> -dclist_prev_node(dclist_head *head, dlist_node *node)
> +dclist_prev_node(const dclist_head *head, const dlist_node *node)
>  {
>  	Assert(head->count > 0);
>  
> @@ -822,7 +822,7 @@ dclist_prev_node(dclist_head *head, dlist_node *node)
>  
>  /* internal support function to get address of head element's struct */
>  static inline void *
> -dclist_head_element_off(dclist_head *head, size_t off)
> +dclist_head_element_off(const dclist_head *head, size_t off)
>  {
>  	Assert(!dclist_is_empty(head));
>  
> @@ -834,7 +834,7 @@ dclist_head_element_off(dclist_head *head, size_t off)
>   *		Return the first node in the list (there must be one).
>   */
>  static inline dlist_node *
> -dclist_head_node(dclist_head *head)
> +dclist_head_node(const dclist_head *head)
>  {
>  	Assert(head->count > 0);
>  
> @@ -843,7 +843,7 @@ dclist_head_node(dclist_head *head)
>  
>  /* internal support function to get address of tail element's struct */
>  static inline void *
> -dclist_tail_element_off(dclist_head *head, size_t off)
> +dclist_tail_element_off(const dclist_head *head, size_t off)
>  {
>  	Assert(!dclist_is_empty(head));
>  
> @@ -854,7 +854,7 @@ dclist_tail_element_off(dclist_head *head, size_t off)
>   * Return the last node in the list (there must be one).
>   */
>  static inline dlist_node *
> -dclist_tail_node(dclist_head *head)
> +dclist_tail_node(const dclist_head *head)
>  {
>  	Assert(head->count > 0);
>  

Dito.


> @@ -988,7 +988,7 @@ slist_has_next(slist_head *head, slist_node *node)
>   * Return the next node in the list (there must be one).
>   */
>  static inline slist_node *
> -slist_next_node(slist_head *head, slist_node *node)
> +slist_next_node(const slist_head *head, const slist_node *node)
>  {
>  	Assert(slist_has_next(head, node));
>  	return node->next;
> @@ -996,7 +996,7 @@ slist_next_node(slist_head *head, slist_node *node)
>  
>  /* internal support function to get address of head element's struct */
>  static inline void *
> -slist_head_element_off(slist_head *head, size_t off)
> +slist_head_element_off(const slist_head *head, size_t off)
>  {
>  	Assert(!slist_is_empty(head));
>  	return (char *) head->head.next - off;
> @@ -1006,7 +1006,7 @@ slist_head_element_off(slist_head *head, size_t off)
>   * Return the first node in the list (there must be one).
>   */
>  static inline slist_node *
> -slist_head_node(slist_head *head)
> +slist_head_node(const slist_head *head)
>  {
>  	return (slist_node *) slist_head_element_off(head, 0);
>  }

Dito.

Greetings,

Andres Freund

  3. Re: [PATCH] Const'ify the arguments of ilist.c/ilist.h functions

    Aleksander Alekseev <aleksander@timescale.com> — 2022-11-23T13:57:36Z

    Hi Andres,

Thanks for the review!

> I don't think it is correct for any of these to add const. The only reason it
> works is because of casting etc.

Fair enough. PFA the corrected patch v2.

-- 
Best regards,
Aleksander Alekseev

  4. Re: [PATCH] Const'ify the arguments of ilist.c/ilist.h functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-07T07:21:26Z

    On 23.11.22 14:57, Aleksander Alekseev wrote:
> Hi Andres,
> 
> Thanks for the review!
> 
>> I don't think it is correct for any of these to add const. The only reason it
>> works is because of casting etc.
> 
> Fair enough. PFA the corrected patch v2.

This patch version looks correct to me.  It is almost the same as the 
one that Andres had posted in his thread, except that yours also 
modifies slist_delete() and dlist_member_check().  Both of these changes 
also look correct to me.

  5. Re: [PATCH] Const'ify the arguments of ilist.c/ilist.h functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-12T07:34:25Z

    On 07.01.23 08:21, Peter Eisentraut wrote:
> On 23.11.22 14:57, Aleksander Alekseev wrote:
>> Hi Andres,
>>
>> Thanks for the review!
>>
>>> I don't think it is correct for any of these to add const. The only 
>>> reason it
>>> works is because of casting etc.
>>
>> Fair enough. PFA the corrected patch v2.
> 
> This patch version looks correct to me.  It is almost the same as the 
> one that Andres had posted in his thread, except that yours also 
> modifies slist_delete() and dlist_member_check().  Both of these changes 
> also look correct to me.

committed

  6. Re: [PATCH] Const'ify the arguments of ilist.c/ilist.h functions

    Andres Freund <andres@anarazel.de> — 2023-01-18T18:22:14Z

    Hi,

On 2023-01-12 08:34:25 +0100, Peter Eisentraut wrote:
> On 07.01.23 08:21, Peter Eisentraut wrote:
> > On 23.11.22 14:57, Aleksander Alekseev wrote:
> > > Hi Andres,
> > > 
> > > Thanks for the review!
> > > 
> > > > I don't think it is correct for any of these to add const. The
> > > > only reason it
> > > > works is because of casting etc.
> > > 
> > > Fair enough. PFA the corrected patch v2.
> > 
> > This patch version looks correct to me.  It is almost the same as the
> > one that Andres had posted in his thread, except that yours also
> > modifies slist_delete() and dlist_member_check().  Both of these changes
> > also look correct to me.
> 
> committed

Unfortunately this causes a build failure with ILIST_DEBUG
enabled. dlist_member_check() uses dlist_foreach(), which isn't set up to work
with const :(.  I'll push a quick workaround.

Greetings,

Andres Freund

  7. Re: [PATCH] Const'ify the arguments of ilist.c/ilist.h functions

    Andres Freund <andres@anarazel.de> — 2023-01-18T18:30:33Z

    Hi,

On 2023-01-18 10:22:14 -0800, Andres Freund wrote:
> On 2023-01-12 08:34:25 +0100, Peter Eisentraut wrote:
> > On 07.01.23 08:21, Peter Eisentraut wrote:
> > > This patch version looks correct to me.  It is almost the same as the
> > > one that Andres had posted in his thread, except that yours also
> > > modifies slist_delete() and dlist_member_check().  Both of these changes
> > > also look correct to me.
> > 
> > committed
> 
> Unfortunately this causes a build failure with ILIST_DEBUG
> enabled. dlist_member_check() uses dlist_foreach(), which isn't set up to work
> with const :(.  I'll push a quick workaround.

Pushed.

Greetings,

Andres Freund