[PATCH 0/5] Assorted small fixes

[PATCH 0/5] Assorted small fixes

[David Gibson]

This series has a number of small fixes to modules which aren't mine.
Most of these were spotted because they trigger warnings with (at
least one version of) clang.

David Gibson (5):
  strmap: Convert to using TCON_WRAP() instead of plain TCON()
  htable: Mark functions constructed by HTABLE_DEFINE_TYPE as UNNEEDED
  idtree: Fix comparison is always false warning
  idtree: Fix misindented statement
  idtree: Fix undefined behaviour (left shift of signed value)

 ccan/cdump/cdump.c                   | 10 ++++-----
 ccan/cdump/cdump.h                   | 10 ++++-----
 ccan/htable/htable_type.h            | 19 +++++++++-------
 ccan/idtree/idtree.c                 |  7 +++---
 ccan/strmap/_info                    |  2 +-
 ccan/strmap/strmap.h                 | 43 ++++++++++++++++--------------------
 ccan/strmap/test/run-iterate-const.c |  4 +---
 ccan/strmap/test/run-order.c         |  4 +---
 ccan/strmap/test/run-prefix.c        | 12 +++++-----
 ccan/strmap/test/run.c               |  4 +---
 tools/ccanlint/ccanlint.c            | 10 ++++-----
 11 files changed, 56 insertions(+), 69 deletions(-)

-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 1/5] strmap: Convert to using TCON_WRAP() instead of plain TCON()

[David Gibson]

The usual way of construction strmap objects is to use the STRMAP_MEMBERS()
macro which expands to both a raw strmap structure and a tcon type canary.
However, the tcon type canary involves a flexible array member which means
that in standard C99 STRMAP_MEMBERS() must appear only at the end of a
structure definition.  But worse, that structure can then only appear at
the end of any other structure it is included in, which is pretty
inconvenient for the intended purpose of creating type specific strmaps.

gcc extensions allow this to work (somehow), but clang complains loudly
about it.

The tcon module already includes the TCON_WRAP() mechanism, which already
provides this same sort of type-specific definitions in a more general way.
So convert strmap (and its users) to that approach.

This removes STRMAP_MEMBERS() entirely, breaking compatibility.  I'm hoping
strmap is used in few enough places that we can get away with that.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/cdump/cdump.c                   | 10 ++++-----
 ccan/cdump/cdump.h                   | 10 ++++-----
 ccan/strmap/_info                    |  2 +-
 ccan/strmap/strmap.h                 | 43 ++++++++++++++++--------------------
 ccan/strmap/test/run-iterate-const.c |  4 +---
 ccan/strmap/test/run-order.c         |  4 +---
 ccan/strmap/test/run-prefix.c        | 12 +++++-----
 ccan/strmap/test/run.c               |  4 +---
 tools/ccanlint/ccanlint.c            | 10 ++++-----
 9 files changed, 41 insertions(+), 58 deletions(-)

diff --git a/ccan/cdump/cdump.c b/ccan/cdump/cdump.c
index f080b96..85c818a 100644
--- a/ccan/cdump/cdump.c
+++ b/ccan/cdump/cdump.c
@@ -213,7 +213,7 @@ static struct cdump_type *get_type(struct cdump_definitions *defs,
 				   enum cdump_type_kind kind,
 				   const char *name)
 {
-	struct cdump_map *m;
+	cdump_map_t *m;
 	struct cdump_type *t;
 
 	switch (kind) {
@@ -609,7 +609,7 @@ static bool tok_take_enum(struct parse_state *ps)
 
 static bool gather_undefines(const char *name,
 			     struct cdump_type *t,
-			     struct cdump_map *undefs)
+			     cdump_map_t *undefs)
 {
 	if (!type_defined(t))
 		strmap_add(undefs, name, t);
@@ -618,15 +618,15 @@ static bool gather_undefines(const char *name,
 
 static bool remove_from_map(const char *name,
 			    struct cdump_type *t,
-			    struct cdump_map *map)
+			    cdump_map_t *map)
 {
 	strmap_del(map, name, NULL);
 	return true;
 }
 
-static void remove_undefined(struct cdump_map *map)
+static void remove_undefined(cdump_map_t *map)
 {
-	struct cdump_map undefs;
+	cdump_map_t undefs;
 
 	/* We can't delete inside iterator, so gather all the undefs
 	 * then remove them. */
diff --git a/ccan/cdump/cdump.h b/ccan/cdump/cdump.h
index 312767b..30822ec 100644
--- a/ccan/cdump/cdump.h
+++ b/ccan/cdump/cdump.h
@@ -50,14 +50,12 @@ struct cdump_type {
 };
 
 /* The map of typenames to definitions */
-struct cdump_map {
-	STRMAP_MEMBERS(struct cdump_type *);
-};
+typedef STRMAP(struct cdump_type *) cdump_map_t;
 
 struct cdump_definitions {
-	struct cdump_map enums;
-	struct cdump_map structs;
-	struct cdump_map unions;
+	cdump_map_t enums;
+	cdump_map_t structs;
+	cdump_map_t unions;
 };
 
 /**
diff --git a/ccan/strmap/_info b/ccan/strmap/_info
index 55319a8..c128cc2 100644
--- a/ccan/strmap/_info
+++ b/ccan/strmap/_info
@@ -29,7 +29,7 @@
  * int main(int argc, char *argv[])
  * {
  * 	size_t i;
- * 	struct { STRMAP_MEMBERS(size_t); } map;
+ * 	STRMAP(size_t) map;
  *
  * 	strmap_init(&map);
  * 	for (i = 1; i < argc; i++)
diff --git a/ccan/strmap/strmap.h b/ccan/strmap/strmap.h
index 8fabc35..0b32e6b 100644
--- a/ccan/strmap/strmap.h
+++ b/ccan/strmap/strmap.h
@@ -21,7 +21,7 @@ struct strmap {
 };
 
 /**
- * STRMAP_MEMBERS - declare members for a type-specific strmap.
+ * STRMAP - declare a type-specific strmap
  * @type: type for this map's values, or void * for any pointer.
  *
  * You use this to create your own typed strmap for a particular type.
@@ -29,14 +29,11 @@ struct strmap {
  * value!
  *
  * Example:
- *	struct strmap_intp {
- *		STRMAP_MEMBERS(int *);
- *	};
+ *	STRMAP(int *) int_strmap;
+ *	strmap_init(&int_strmap);
  */
-#define STRMAP_MEMBERS(type)			\
-	struct strmap raw;			\
-	TCON(type canary)
-
+#define STRMAP(type)				\
+	TCON_WRAP(struct strmap, type canary)
 
 /**
  * strmap_init - initialize a string map (empty)
@@ -46,11 +43,11 @@ struct strmap {
  * need this.
  *
  * Example:
- *	struct strmap_intp map;
+ *	STRMAP(int *) map;
  *
  *	strmap_init(&map);
  */
-#define strmap_init(map) strmap_init_(&(map)->raw)
+#define strmap_init(map) strmap_init_(tcon_unwrap(map))
 
 static inline void strmap_init_(struct strmap *map)
 {
@@ -65,7 +62,7 @@ static inline void strmap_init_(struct strmap *map)
  *	if (!strmap_empty(&map))
  *		abort();
  */
-#define strmap_empty(map) strmap_empty_(&(map)->raw)
+#define strmap_empty(map) strmap_empty_(tcon_unwrap(map))
 
 static inline bool strmap_empty_(const struct strmap *map)
 {
@@ -85,7 +82,7 @@ static inline bool strmap_empty_(const struct strmap *map)
  *		printf("hello => %i\n", *val);
  */
 #define strmap_get(map, member) \
-	tcon_cast((map), canary, strmap_get_(&(map)->raw, (member)))
+	tcon_cast((map), canary, strmap_get_(tcon_unwrap(map), (member)))
 void *strmap_get_(const struct strmap *map, const char *member);
 
 /**
@@ -106,8 +103,8 @@ void *strmap_get_(const struct strmap *map, const char *member);
  *	if (!strmap_add(&map, "goodbye", val))
  *		printf("goodbye was already in the map\n");
  */
-#define strmap_add(map, member, value)				\
-	strmap_add_(&tcon_check((map), canary, (value))->raw,	\
+#define strmap_add(map, member, value)					\
+	strmap_add_(tcon_unwrap(tcon_check((map), canary, (value))),	\
 		    (member), (void *)(value))
 
 bool strmap_add_(struct strmap *map, const char *member, const void *value);
@@ -130,7 +127,7 @@ bool strmap_add_(struct strmap *map, const char *member, const void *value);
  *		printf("goodbye was not in the map?\n");
  */
 #define strmap_del(map, member, valuep)					\
-	strmap_del_(&tcon_check_ptr((map), canary, valuep)->raw,	\
+	strmap_del_(tcon_unwrap(tcon_check_ptr((map), canary, valuep)), \
 		    (member), (void **)valuep)
 char *strmap_del_(struct strmap *map, const char *member, void **valuep);
 
@@ -143,7 +140,7 @@ char *strmap_del_(struct strmap *map, const char *member, void **valuep);
  * Example:
  *	strmap_clear(&map);
  */
-#define strmap_clear(map) strmap_clear_(&(map)->raw)
+#define strmap_clear(map) strmap_clear_(tcon_unwrap(map))
 
 void strmap_clear_(struct strmap *map);
 
@@ -160,9 +157,7 @@ void strmap_clear_(struct strmap *map);
  * You should not alter the map within the @handle function!
  *
  * Example:
- *	struct strmap_intp {
- *		STRMAP_MEMBERS(int *);
- *	};
+ *	typedef STRMAP(int *) strmap_intp;
  *	static bool dump_some(const char *member, int *value, int *num)
  *	{
  *		// Only dump out num nodes.
@@ -172,7 +167,7 @@ void strmap_clear_(struct strmap *map);
  *		return true;
  *	}
  *
- *	static void dump_map(const struct strmap_intp *map)
+ *	static void dump_map(const strmap_intp *map)
  *	{
  *		int max = 100;
  *		strmap_iterate(map, dump_some, &max);
@@ -181,7 +176,7 @@ void strmap_clear_(struct strmap *map);
  *	}
  */
 #define strmap_iterate(map, handle, arg)				\
-	strmap_iterate_(&(map)->raw,					\
+	strmap_iterate_(tcon_unwrap(map),				\
 			typesafe_cb_cast(bool (*)(const char *,		\
 						  void *, void *),	\
 					 bool (*)(const char *,		\
@@ -202,7 +197,7 @@ void strmap_iterate_(const struct strmap *map,
  * strmap_empty() on the returned pointer.
  *
  * Example:
- *	static void dump_prefix(const struct strmap_intp *map,
+ *	static void dump_prefix(const strmap_intp *map,
  *				const char *prefix)
  *	{
  *		int max = 100;
@@ -214,10 +209,10 @@ void strmap_iterate_(const struct strmap *map,
  */
 #if HAVE_TYPEOF
 #define strmap_prefix(map, prefix) \
-	((const __typeof__(map))strmap_prefix_(&(map)->raw, (prefix)))
+	((const __typeof__(map))strmap_prefix_(tcon_unwrap(map), (prefix)))
 #else
 #define strmap_prefix(map, prefix) \
-	((const void *)strmap_prefix_(&(map)->raw, (prefix)))
+	((const void *)strmap_prefix_(tcon_unwrap(map), (prefix)))
 #endif
 
 const struct strmap *strmap_prefix_(const struct strmap *map,
diff --git a/ccan/strmap/test/run-iterate-const.c b/ccan/strmap/test/run-iterate-const.c
index 07c814a..63bea95 100644
--- a/ccan/strmap/test/run-iterate-const.c
+++ b/ccan/strmap/test/run-iterate-const.c
@@ -14,9 +14,7 @@ static bool find_string(const char *str, char *member, const char *cmp)
 
 int main(void)
 {
-	struct strmap_charp {
-		STRMAP_MEMBERS(char *);
-	} map;
+	STRMAP(char *) map;
 
 	plan_tests(3);
 
diff --git a/ccan/strmap/test/run-order.c b/ccan/strmap/test/run-order.c
index 68062ea..b541666 100644
--- a/ccan/strmap/test/run-order.c
+++ b/ccan/strmap/test/run-order.c
@@ -33,9 +33,7 @@ static bool dump(const char *member, char *value, bool *ok)
 
 int main(void)
 {
-	struct strmap_charp {
-		STRMAP_MEMBERS(char *);
-	} map;
+	STRMAP(char *) map;
 	unsigned int i;
 	char *str[NUM];
 	bool dump_ok;
diff --git a/ccan/strmap/test/run-prefix.c b/ccan/strmap/test/run-prefix.c
index 9c5156e..d88eb55 100644
--- a/ccan/strmap/test/run-prefix.c
+++ b/ccan/strmap/test/run-prefix.c
@@ -24,11 +24,9 @@ static bool find_empty(const char *index, char *value, char *empty)
 
 int main(void)
 {
-	struct map {
-		STRMAP_MEMBERS(char *);
-	};
-	struct map map;
-	const struct map *sub;
+	typedef STRMAP(char *) map_t;
+	map_t map;
+	const map_t *sub;
 	unsigned int i;
 	char *str[NUM], *empty;
 
@@ -56,9 +54,9 @@ int main(void)
 
 	/* Everything */
 	sub = strmap_prefix(&map, "0");
-	ok1(sub->raw.u.n == map.raw.u.n);
+	ok1(tcon_unwrap(sub)->u.n == tcon_unwrap(&map)->u.n);
 	sub = strmap_prefix(&map, "");
-	ok1(sub->raw.u.n == map.raw.u.n);
+	ok1(tcon_unwrap(sub)->u.n == tcon_unwrap(&map)->u.n);
 
 	/* Single. */
 	sub = strmap_prefix(&map, "00000000");
diff --git a/ccan/strmap/test/run.c b/ccan/strmap/test/run.c
index aaa8618..f64ddc5 100644
--- a/ccan/strmap/test/run.c
+++ b/ccan/strmap/test/run.c
@@ -4,9 +4,7 @@
 
 int main(void)
 {
-	struct strmap_charp {
-		STRMAP_MEMBERS(char *);
-	} map;
+	STRMAP(char *) map;
 	const char str[] = "hello";
 	const char val[] = "there";
 	const char none[] = "";
diff --git a/tools/ccanlint/ccanlint.c b/tools/ccanlint/ccanlint.c
index 158195c..1e5cf27 100644
--- a/tools/ccanlint/ccanlint.c
+++ b/tools/ccanlint/ccanlint.c
@@ -35,12 +35,10 @@
 #include <ccan/tal/path/path.h>
 #include <ccan/strmap/strmap.h>
 
-struct ccanlint_map {
-	STRMAP_MEMBERS(struct ccanlint *);
-};
+typedef STRMAP(struct ccanlint *) ccanlint_map_t;
 
 int verbose = 0;
-static struct ccanlint_map tests;
+static ccanlint_map_t tests;
 bool safe_mode = false;
 bool keep_results = false;
 bool non_ccan_deps = false;
@@ -273,7 +271,7 @@ static bool init_deps(const char *member, struct ccanlint *c, void *unused)
 }
 
 static bool check_names(const char *member, struct ccanlint *c,
-			struct ccanlint_map *names)
+			ccanlint_map_t *names)
 {
 	if (!strmap_add(names, c->name, c))
 		err(1, "Duplicate name %s", c->name);
@@ -282,7 +280,7 @@ static bool check_names(const char *member, struct ccanlint *c,
 
 static void init_tests(void)
 {
-	struct ccanlint_map names;
+	ccanlint_map_t names;
 	struct ccanlint **table;
 	size_t i, num;
 
-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 2/5] htable: Mark functions constructed by HTABLE_DEFINE_TYPE as UNNEEDED

[David Gibson]

The HTABLE_DEFINE_TYPE macro builds a type-specific hash table by
constructing a bunch of simple wrapper functions.  The user of the hash
table may not end up using all of these.  With gcc the fact that the
functions are inline stops an warnings about unused functions, but that's
not the case with clang.

Suppress these warnings by marking all the constructed functions except
for name##_add() as UNNEEDED (using the macro from ccan/compiler).  _add
is left alone on the grounds that a hash table you never add anything to
isn't much use, so this will help you to spot an entirely redundant
HTABLE_DEFINE_TYPE invocation.  *_init() would be a more obvious choice,
except that there is both *_init() and *_init_sized() and you only need
to use one of these.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/htable/htable_type.h | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/ccan/htable/htable_type.h b/ccan/htable/htable_type.h
index ad3974c..6764c3f 100644
--- a/ccan/htable/htable_type.h
+++ b/ccan/htable/htable_type.h
@@ -2,6 +2,7 @@
 #ifndef CCAN_HTABLE_TYPE_H
 #define CCAN_HTABLE_TYPE_H
 #include <ccan/htable/htable.h>
+#include <ccan/compiler/compiler.h>
 #include "config.h"
 
 /**
@@ -50,15 +51,16 @@
 	{								\
 		return hashfn(keyof((const type *)elem));		\
 	}								\
-	static inline void name##_init(struct name *ht)			\
+	static inline UNNEEDED void name##_init(struct name *ht)	\
 	{								\
 		htable_init(&ht->raw, name##_hash, NULL);		\
 	}								\
-	static inline void name##_init_sized(struct name *ht, size_t s)	\
+	static inline UNNEEDED void name##_init_sized(struct name *ht,	\
+						      size_t s)		\
 	{								\
 		htable_init_sized(&ht->raw, name##_hash, NULL, s);	\
 	}								\
-	static inline void name##_clear(struct name *ht)		\
+	static inline UNNEEDED void name##_clear(struct name *ht)	\
 	{								\
 		htable_clear(&ht->raw);					\
 	}								\
@@ -66,11 +68,12 @@
 	{								\
 		return htable_add(&ht->raw, hashfn(keyof(elem)), elem);	\
 	}								\
-	static inline bool name##_del(struct name *ht, const type *elem) \
+	static inline UNNEEDED bool name##_del(struct name *ht,		\
+					       const type *elem)	\
 	{								\
 		return htable_del(&ht->raw, hashfn(keyof(elem)), elem);	\
 	}								\
-	static inline type *name##_get(const struct name *ht,		\
+	static inline UNNEEDED type *name##_get(const struct name *ht,	\
 				       const HTABLE_KTYPE(keyof) k)	\
 	{								\
 		/* Typecheck for eqfn */				\
@@ -81,7 +84,7 @@
 				  (bool (*)(const void *, void *))(eqfn), \
 				  k);					\
 	}								\
-	static inline bool name##_delkey(struct name *ht,		\
+	static inline UNNEEDED bool name##_delkey(struct name *ht,	\
 					 const HTABLE_KTYPE(keyof) k)	\
 	{								\
 		type *elem = name##_get(ht, k);				\
@@ -89,12 +92,12 @@
 			return name##_del(ht, elem);			\
 		return false;						\
 	}								\
-	static inline type *name##_first(const struct name *ht,		\
+	static inline UNNEEDED type *name##_first(const struct name *ht, \
 					 struct name##_iter *iter)	\
 	{								\
 		return htable_first(&ht->raw, &iter->i);		\
 	}								\
-	static inline type *name##_next(const struct name *ht,		\
+	static inline UNNEEDED type *name##_next(const struct name *ht,	\
 					struct name##_iter *iter)	\
 	{								\
 		return htable_next(&ht->raw, &iter->i);			\
-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 3/5] idtree: Fix comparison is always false warning

[David Gibson]

idtree.c:146 triggers a "comparison is always false" warning on some
compiler configurations, since the 'id' variable is unsigned.

Elsewhere in the module ids seem to be represented by (signed) ints, so
use the same convention here, suppressing the warning and also maybe being
more correct in other ways.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/idtree/idtree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ccan/idtree/idtree.c b/ccan/idtree/idtree.c
index e887392..48add6a 100644
--- a/ccan/idtree/idtree.c
+++ b/ccan/idtree/idtree.c
@@ -100,7 +100,8 @@ static int sub_alloc(struct idtree *idp, const void *ptr, int *starting_id)
 	int n, m, sh;
 	struct idtree_layer *p, *pn;
 	struct idtree_layer *pa[MAX_LEVEL+1];
-	unsigned int l, id, oid;
+	unsigned int l;
+	int id, oid;
 	uint32_t bm;
 
 	memset(pa, 0, sizeof(pa));
-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 4/5] idtree: Fix misindented statement

[David Gibson]

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/idtree/idtree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ccan/idtree/idtree.c b/ccan/idtree/idtree.c
index 48add6a..5bd8882 100644
--- a/ccan/idtree/idtree.c
+++ b/ccan/idtree/idtree.c
@@ -135,7 +135,7 @@ restart:
 			 */
 			sh = IDTREE_BITS * (l + 1);
 			if (oid >> sh == id >> sh)
-			continue;
+				continue;
 			else
 				goto restart;
 		}
-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 5/5] idtree: Fix undefined behaviour (left shift of signed value)

[David Gibson]

~0 will be signed and negative on any 2s complement system, and
left shifting a negative value has undefined behaviour in C.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/idtree/idtree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ccan/idtree/idtree.c b/ccan/idtree/idtree.c
index 5bd8882..6e75450 100644
--- a/ccan/idtree/idtree.c
+++ b/ccan/idtree/idtree.c
@@ -278,7 +278,7 @@ void *idtree_lookup(const struct idtree *idp, int id)
 	 * present.  If so, tain't one of ours!
 	 */
 	if (n + IDTREE_BITS < 31 &&
-	    (id & ~(~0 << MAX_ID_SHIFT)) >> (n + IDTREE_BITS))
+	    (id & ~(~0U << MAX_ID_SHIFT)) >> (n + IDTREE_BITS))
 	     return NULL;
 
 	/* Mask off upper bits we don't use for the search. */
-- 
2.5.0

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 0/5] Assorted small fixes

[Rusty Russell]

David Gibson <david@gibson.dropbear.id.au> writes:
> This series has a number of small fixes to modules which aren't mine.
> Most of these were spotted because they trigger warnings with (at
> least one version of) clang.

Thanks, please apply them all.

Cheers,
Rusty.
_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan