Re: embedded list v3

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(at)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