Re: embedded list v3 - Mailing list pgsql-hackers

Hi Peter,

On Monday, October 08, 2012 09:43:51 PM Peter Geoghegan wrote:
> Pendantry: This should be in alphabetical order:
> 
> ! OBJS = stringinfo.o ilist.o
Argh. Youve said that before. Somehow I reintroduced it...

> I notice that the patch (my revision) produces a whole bunch of
> warnings like this with Clang, though not GCC:
> 
> """"""""
> 
> [peter@peterlaptop cache]$ clang -O2 -g -Wall -Wmissing-prototypes
> -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -fexcess-precision=standard -g -I../../../../src/include
> -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -I/usr/include/libxml2   -c -o
> catcache.o catcache.c -MMD -MP -MF .deps/catcache.Po
> clang: warning: argument unused during compilation:
> '-fexcess-precision=standard'
> catcache.c:457:21: warning: expression result unused [-Wunused-value]
>                 CatCache   *ccp = slist_container(CatCache, cc_next,
> cache_iter.cur);
> 
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../../../src/include/lib/ilist.h:721:3: note: expanded from macro
> 'slist_container'
>         (AssertVariableIsOfTypeMacro(ptr, slist_node *),...
>          ^
> ../../../../src/include/c.h:735:2: note: expanded from macro
> 'AssertVariableIsOfTypeMacro'
>         StaticAssertExpr(__builtin_types_compatible_p(__typeof__(varname),
> typename), \
>         ^
> ../../../../src/include/c.h:710:46: note: expanded from macro
> 'StaticAssertExpr' ({ StaticAssertStmt(condition, errmessage); true; })
>                                                     ^
> ../../../../src/include/c.h:185:15: note: expanded from macro 'true'
> #define true    ((bool) 1)
>                  ^      ~
> 
> *** SNIP SNIP SNIP ***
> 
> catcache.c:1818:21: warning: expression result unused [-Wunused-value]
>                 CatCache   *ccp = slist_container(CatCache, cc_next,
> iter.cur); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../../../src/include/lib/ilist.h:722:3: note: expanded from macro
> 'slist_container'
>          AssertVariableIsOfTypeMacro(((type*)NULL)->membername,
> slist_node),     \
>          ^
> ../../../../src/include/c.h:735:2: note: expanded from macro
> 'AssertVariableIsOfTypeMacro'
>         StaticAssertExpr(__builtin_types_compatible_p(__typeof__(varname),
> typename), \
>         ^
> ../../../../src/include/c.h:710:46: note: expanded from macro
> 'StaticAssertExpr' ({ StaticAssertStmt(condition, errmessage); true; })
>                                                     ^
> ../../../../src/include/c.h:185:15: note: expanded from macro 'true'
> #define true    ((bool) 1)
>                  ^      ~
> 28 warnings generated.
> 
> """"""""
> 
> I suppose that this is something that's going to have to be addressed
> sooner or later.
That looks like an annoying warning. Split of StaticAssertExpr into 
StaticAssertExpr and StaticAssertExprBool?

> This seems kind of arbitrary:
> 
>   /* The socket number we are listening for connections on */
>   int            PostPortNumber;
> +
>   /* The directory names for Unix socket(s) */
>   char       *Unix_socket_directories;
> +
>   /* The TCP listen address(es) */
>   char       *ListenAddresses;
Yep, no idea why I added the spaces.

> This looks funny:
> 
> + #endif   /* defined(USE_INLINE) ||
> +                                  * defined(ILIST_DEFINE_FUNCTIONS) */
> 
> I tend to consider the 80-column thing a recommendation more than a
> requirement.
Thats pgindent's doing. I couldn't convince it not to do that short of making 
it a multiline comment with ----'s.

> A further stylistic gripe is that this:
> 
> + #define dlist_container(type, membername, ptr)                                 
\
> +     (AssertVariableIsOfTypeMacro(ptr, dlist_node *),                         \
> +      AssertVariableIsOfTypeMacro(((type *) NULL)->membername, dlist_node),    
> \ +      ((type *)((char *)(ptr) - offsetof(type, membername)))                 \
> +      )
> 
> Should probably look like this:
> 
> + #define dlist_container(type, membername, ptr)                                 
\
> +     (AssertVariableIsOfTypeMacro(ptr, dlist_node *),                         \
> +      AssertVariableIsOfTypeMacro(((type *) NULL)->membername, dlist_node),    
> \ +      ((type *)((char *)(ptr) - offsetof(type, membername))))
Why? I find the former better readable.

> This is a little unclear:
> 
> +  * dlist_foreach (iter, &db->tables)
> +  * {
> +  *       // inside an *_foreach the iterator's .cur field can be used to
> access +  *       // the current element

> This comment:
> 
> +  * Singly linked lists are *not* circularly linked (how could they be?)
> in +  * contrast to the doubly linked lists. As no pointer to the last
> list element
> 
> Isn't quite accurate, if I've understood you correctly; it is surely
> possible in principle to have a circular singly-linked list.
Its complete crap. One shouldn't write comments after a 2h delayed 6h train 
ride. No idea what exactly warped my mind there.

> This could be a little clearer; its "content"?:
> 
> +  * This is used to convert a dlist_node* returned by some list
> +  * navigation/manipulation back to its content.
Youre right.'containing element'? 'containing struct'?

> I don't think you should refer to --enable-cassert here; it is
> better-principled to refer to USE_ASSERT_CHECKING:
Fine with me.

> That's all for now.
Thanks.

Greetings,

Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services