["John Dill" <John.Dill@xxxxxxxxxxxxxxxxx>]

Title: adding units

I'm tinkering with the 1.10.6 source code and I'm wondering if there's
any opinions about the position and placement of units when using the
different 'display' enumerations.

Here are my thoughts based on one particular use case.
(my comments are in [])

\code snippet
static const char *
hfinfo_uint_value_format(const header_field_info *hfinfo)
{
  const char *format = NULL;

  /* Pick the proper format string */
  if (hfinfo->type == FT_FRAMENUM) {
    /*
     * Frame numbers are always displayed in decimal.
     */
    format = "%u"; [no reason to apply a units field to FT_FRAMENUM imo]
  } else {
    switch (hfinfo->display) {
      case BASE_DEC:
        format = "%u"; [would become "%u %s", should be a no-brainer]
        break;
      case BASE_DEC_HEX:
        switch (hfinfo->type) {
          case FT_UINT8:
            format = "%u (0x%02x)";
            [could become "%u %s (0x%02x)" or "%u (0x%02x) %s", prefer first version]
            break;
          case FT_UINT16:
            format = "%u (0x%04x)"; [prefer "%u %s (0x%04x)"]
            break;
          case FT_UINT24:
            format = "%u (0x%06x)"; [prefer "%u %s (0x%06x)"]
            break;
          case FT_UINT32:
            format = "%u (0x%08x)"; [prefer "%u %s (0x%08x)"]
            break;
          default:
            DISSECTOR_ASSERT_NOT_REACHED();
            ;
        }
        break;
      case BASE_OCT:
        format = "%#o"; [my preference is not to include units for octal display]
        break;
      case BASE_HEX:
        switch (hfinfo->type) {
          case FT_UINT8:
            [my preference is to not include units for hexadecimal display, since
             the format tends to be used to display raw data]
            format = "0x%02x";
            break;
          case FT_UINT16:
            format = "0x%04x";
            break;
          case FT_UINT24:
            format = "0x%06x";
            break;
          case FT_UINT32:
            format = "0x%08x";
            break;
          default:
            DISSECTOR_ASSERT_NOT_REACHED();
            ;
        }
        break;
     case BASE_HEX_DEC:
        switch (hfinfo->type) {
          case FT_UINT8:
            format = "0x%02x (%u)";
            [my preference in this instance is to attach units to the decimal
             part of the display, so you would have "0x%02x (%u %s)"]
            break;
          case FT_UINT16:
            format = "0x%04x (%u)"; [prefer "0x%04x (%u %s)"]
            break;
          case FT_UINT24:
            format = "0x%06x (%u)"; [prefer "0x%06x (%u %s)"]
            break;
          case FT_UINT32:
            format = "0x%08x (%u)"; [prefer "0x%08x (%u %s)"]
            break;
          default:
            DISSECTOR_ASSERT_NOT_REACHED();
            ;
        }
        break;
      default:
        DISSECTOR_ASSERT_NOT_REACHED();
        ;
    }
  }
  return format;
}
\endcode

Obviously this would require some kind of change in 'proto_custom_set'
and 'fill_label_bitfield' where this type of function is used, since the
order of the printf modifiers change based on the 'display' enumeration
value.

Currently, I'm using the next available bit above BASE_EXT_STRING to
distinguish the conditions as BASE_UNIT_STRING.

The FT_FLOAT and FT_DOUBLE types should be as simple as adding the unit
string after the value in 'proto_custom_set' and 'proto_item_fill_label'.

It appears that some error condition checking happens in 'tmp_fld_check_assert'.
If I detect a bad combination of a FT_TYPE and BASE_UNIT_STRING, should I
try to report that as an error somehow, or ignore it?

Any comments on the placement of unit strings in the format, or other
considerations I should look into.

Thanks,
John Dill