Re: embedded list v3

On 29 September 2012 01:14, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Current version is available at branch ilist in:
> git://git.postgresql.org/git/users/andresfreund/postgres.git
> ssh://git(at)git(dot)postgresql(dot)org/users/andresfreund/postgres.git

I have taken a look at the ilist branch, merged into today's master.
This patch includes changes to core, so that certain call sites now
call this new doubly-linked list infrastructure rather than
infrastructure located in dllist.c, which the patch deprecates, per
Andres' proprosal. As a convenience to others, I attach a patch file
of same. I have taken the liberty of altering it so that it now uses
the static assert infrastructure that Tom split off, altered somewhat,
and committed about a week ago, with commits
ea473fb2dee7dfe61f8fbdfac9d9da87d84e564e and
0d0aa5d29175c539db1981be27dbbf059be6f3b1. I haven't altered anything
beyond what is needed to make the patch build against head, including
changes already suggested by Tom; this is still entirely Andres'
patch.

Pendantry: This should be in alphabetical order:

! OBJS = stringinfo.o ilist.o

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.

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;

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.

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

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.

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.

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

+ /*
+ * enable for extra debugging. This is rather expensive, so it's not
enabled by
+ * default even with --enable-cassert.
+ */
+ /* #define ILIST_DEBUG */

As to the substantial issues, I have no strong opinion either way as
to whether or not the doubly-linked list should be circular. It might
be nice to see some numbers, if that's what is informing Andres'
thinking here.

The code appears to be reasonable to me. The example is generally illustrative.

That's all for now.

--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services