Wireshark-dev: Re: [Wireshark-dev] RFC: sorted value_string + bsearch

From: Jakub Zawadzki <darkjames@xxxxxxxxxxxxxxxx>
Date: Tue, 27 Apr 2010 13:29:12 +0200
On Tue, Apr 27, 2010 at 12:34:14PM +0200, Anders Broman wrote:
> Committed revision 32574 to faciliate further testing, dockumentation updates will be needed if/when we are
> happy with the implementation.

Thx.

I have two changes in implementation :)
 - use function pointers instead of switch.
 - initalize ->match_type during first use of match_str() func.

I remove IS_INT_TYPE macro, cause it's now unused.

but it could be good idea to replace:
 (hfinfo->type == FT_UINT8 ||
  hfinfo->type == FT_UINT16 ||
  hfinfo->type == FT_UINT24 ||
  hfinfo->type == FT_UINT32 ||
  hfinfo->type == FT_UINT64 ||
  hfinfo->type == FT_INT8 ||
  hfinfo->type == FT_INT16 ||
  hfinfo->type == FT_INT24 ||
  hfinfo->type == FT_INT32 ||
  hfinfo->type == FT_INT64)

with single IS_[U]INT_TYPE...
diff --git epan/proto.c epan/proto.c
index 2d6761a..6d22b50 100644
--- epan/proto.c
+++ epan/proto.c
@@ -4393,39 +4393,12 @@ static void tmp_fld_check_assert(header_field_info *hfinfo) {
 	}
 }
 
-static void
-proto_reinit_value_string(value_string_ext *vse)
-{
-	const value_string *vals = vse->vals;
-	int type = VS_INDEX;
-	guint32 prev = 0;
-	guint i;
-
-	for (i = 0; i < vse->length; i++) {
-		if (type == VS_INDEX && vals[i].value != i)
-			type = VS_BIN_TREE;
-
-		if (type == VS_BIN_TREE && prev > vals[i].value) {
-			type = VS_SEARCH;
-			break;
-		}
-
-		prev = vals[i].value;
-	}
-
-	vse->match_type = type;
-	printf("%p: %d\n", vse, type);
-}
-
 static int
 proto_register_field_init(header_field_info *hfinfo, const int parent)
 {
 
 	tmp_fld_check_assert(hfinfo);
 
-	if (hfinfo->strings && (hfinfo->display & BASE_EXT_STRING) && IS_INT_TYPE(hfinfo->type))
-		proto_reinit_value_string((value_string_ext *) hfinfo->strings);
-
 	/* if this is a bitfield, compute bitshift */
 	if (hfinfo->bitmask) {
 		hfinfo->bitshift = wrs_count_bitshift(hfinfo->bitmask);
diff --git epan/proto.h epan/proto.h
index 97b934d..49394eb 100644
--- epan/proto.h
+++ epan/proto.h
@@ -159,10 +159,6 @@ typedef enum {
 	BASE_CUSTOM	/**< call custom routine (in ->strings) to format */
 } base_display_e;
 
-#define IS_INT_TYPE(type) \
-  ((type) == FT_UINT8 || (type) == FT_UINT16 || (type) == FT_UINT24 || (type) == FT_UINT32 || (type) == FT_UINT64 || \
-   (type) == FT_INT8 || (type) == FT_INT16 || (type) == FT_INT24 || (type) == FT_INT32 || (type) == FT_INT64)
-
 /* Following constants have to be ORed with a base_display_e when dissector
  * want to use specials MACROs (for the moment, only RVALS) for a
  * header_field_info */
diff --git epan/value_string.c epan/value_string.c
index 44b0647..a2b45f9 100644
--- epan/value_string.c
+++ epan/value_string.c
@@ -120,39 +120,91 @@ match_strval(const guint32 val, const value_string *vs) {
     return match_strval_idx(val, vs, &ignore_me);
 }
 
-const gchar*
-match_strval_ext(const guint32 val, const value_string_ext *vs) {
+static const gchar *
+_match_strval_linear(const guint32 val, const value_string_ext *vs)
+{
+  return match_strval(val, vs->vals);
+}
+
+static const gchar *
+_match_strval_index(const guint32 val, const value_string_ext *vs)
+{
+  return (val < vs->length) ? vs->vals[val].strptr : NULL;
+}
+
+static const gchar *
+_match_strval_bsearch(const guint32 val, const value_string_ext *vs)
+{
   guint low, idx, max;
   guint32 item;
-  if(vs) {
-    switch(vs->match_type){
-    case VS_DEFAULT: 
-      /* XXX: reinit? */
-    case VS_SEARCH:
-      return match_strval(val, vs->vals);
-    case VS_INDEX:
-      return (val < vs->length) ? vs->vals[val].strptr : NULL;
-    case VS_BIN_TREE:
-      for (low = 0, max = vs->length; low < max; ) {
-        idx = (low + max) / 2;
-        item = vs->vals[idx].value;
-
-        if (val < item)
-          max = idx;
-        else if (val > item)
-         low = idx + 1;
-        else
-          return vs->vals[idx].strptr;
-      }
-      break;
-    default:
-      g_assert_not_reached();
+
+  for (low = 0, max = vs->length; low < max; ) {
+    idx = (low + max) / 2;
+    item = vs->vals[idx].value;
+
+    if (val < item)
+      max = idx;
+    else if (val > item)
+      low = idx + 1;
+    else
+      return vs->vals[idx].strptr;
+  }
+  return NULL;
+}
+
+const gchar *
+match_strval_ext_init(const guint32 val, value_string_ext *vse)
+{
+  const value_string *vals = vse->vals;
+
+/* The way matching of value is done in a value_string:
+ * 0 default, value will be set in proto_register_field_init()
+ * 1 Sequential search (as in a normal value string)
+ * 2 The value used as an index(the value string MUST have all values 0-max defined)
+ * 3 Binary search, the valuse MUST be in numerical order.
+ */
+  enum { VS_SEARCH = 0, VS_INDEX, VS_BIN_TREE } type = VS_INDEX;
+
+  guint32 prev = 0;
+  guint i;
+
+  for (i = 0; i < vse->length; i++) {
+    if (type == VS_INDEX && vals[i].value != i)
+      type = VS_BIN_TREE;
+
+    if (type == VS_BIN_TREE && prev > vals[i].value) {
+      type = VS_SEARCH;
       break;
     }
+
+    prev = vals[i].value;
   }
-  return NULL;
+  
+  switch (type) {
+  case VS_SEARCH:
+    vse->match = _match_strval_linear;
+    break;
+  case VS_INDEX:
+    vse->match = _match_strval_index;
+    break;
+  case VS_BIN_TREE:
+    vse->match = _match_strval_bsearch;
+    break;
+  default:
+    g_assert_not_reached();
+    break;
+  }
+
+  printf("%p: %d\n", vse, type);
+  return vse->match(val, vse);
 }
 
+const gchar*
+match_strval_ext(const guint32 val, const value_string_ext *vs) {
+    if (vs)
+      return vs->match(val, vs);
+    return NULL;
+}
 
 /* Tries to match val against each element in the value_string array vs.
    Returns the associated string ptr on a match.
diff --git epan/value_string.h epan/value_string.h
index 677ebb6..aa73ba0 100644
--- epan/value_string.h
+++ epan/value_string.h
@@ -34,24 +34,17 @@ typedef struct _value_string {
   const gchar   *strptr;
 } value_string;
 
-/* The way matching of value is done in a value_string:
- * 0 default, value will be set in proto_register_field_init()
- * 1 Sequential search (as in a normal value string)
- * 2 The value used as an index(the value string MUST have all values 0-max defined)
- * 3 Binary search, the valuse MUST be in numerical order.
- */
-#define VS_DEFAULT  0 
-#define VS_SEARCH   1
-#define VS_INDEX    2
-#define VS_BIN_TREE 3
+struct _value_string_ext;
+typedef const char *(*value_string_match_t)(const guint32, const struct _value_string_ext *);
 
-typedef struct {
-  guint match_type;             /* One of the values abowe */
+typedef struct _value_string_ext {
+  value_string_match_t match;
   guint length;                 /* length of the array */
   const value_string *vals;     /* the value string */
 } value_string_ext;
 
-#define VALUE_STRING_EXT_INIT(x) { VS_DEFAULT, array_length(x)-1, x }
+const gchar *match_strval_ext_init(const guint32 val, value_string_ext *vse);
+#define VALUE_STRING_EXT_INIT(x) { (value_string_match_t) match_strval_ext_init, array_length(x)-1, x }
 
 /* Struct for the str_to_str, match_strstr_idx, and match_strstr functions */