On Jan 12, 2007, at 2:02 PM, Sebastien Tandel wrote:
As I was not totally satisfied by my solution, I'm not totally with
your
neither. With VALS and TFS (and, in fact, also NULL :)), as you said,
this field 'display' is usually defined to know which base to use
(dec,
hex, dec_hex, ...) to display a value. If you want a range_string
displayed with base dec, hex, ... you have to duplicate
BASE_RANGE_STRING values as for example BASE_RANGE_STRING_DEC,
BASE_RANGE_STRING_HEX, ...
...or have a BASE_RANGE_STRING flag that you OR with BASE_xxx.
That means that the value in the structure isn't one of the enum
values, so the debugger won't be able to prettyprint it, though.
VALS and TFS are independent from the base
chosen (or in general display field) and RVALS won't be.
What do you think of an hybrid solution between mine and your defining
an additional field (i.e. convertfield_type) in head_register_info
which
identifies which type of structure is in CONVERTFIELD (strings)?
- It would bring the independence from the base avoiding us to
duplicate the fields (BASE_RANGE_STRING_DEC,
BASE_RANGE_STRING_HEX, ...)
- this field could be hidden by the HFILL macro when you don't have to
use it (and use another, HFILL2(?), when you have to use this field)
We might want to define macros to expand to definitions of fields of
particular types, with particular options; that'd also handle the
HFILL stuff, without having to have multiple HFILL macros. Keep the
old HFILL and have it initialize that additional field to a default
value, and use the macros to create definitions that set it (and
convert existing definitions to use the new macros over time as
desired).
- it would be scalable for the current and future implementation if
you
create another structure à la range_string without changing anything
in
the code of the existing dissectors.
...which means preserving binary compatibility as well.
- would be possible to use FT_BOOLEAN as well
- it's clean (from the syntax point of view and from the semantic as
well) :)
Adding an additional field *does* make the structure a bit bigger,
which might worsen the memory and cache footprint of Wireshark;
whether it'd make a significant difference is another matter.
Note also that other display options might be useful to attach to
fields, e.g. units, so you could define something such as
HF_UINT_WITH_UNITS(hf_xxx_data_rate, "Data rate", "xxx.data_rate",
FT_UINT64, BASE_DEC, NULL, 0x0, "", "bytes/s")
(as an example of the new type of macro I mentioned), which might
expand to something such as
{ &hf_xxx_data_rate,
{ "Data rate", "xxx.data_rate", FT_UINT64, BASE_DEC, NULL, 0x0, "Data
rate, in bytes/s", "bytes/s", HFILL }},
so that the field would be displayed as "Data rate: %u bytes/s" rather
than just "Data rate: %u". That might let us get rid of a number of
proto_tree_add_xxx_format() calls. (Arguably, every time somebody
calls proto_tree_add_xxx_format(), it means we failed to anticipate a
need, although some might be sufficiently infrequently used that it's
not worth adding another display feature for it.)
I'm not sure whether there's a way to, for example, bundle all the
display information in a single structure, e.g. one with an optional
value_string or true_false_string or range_string table *and* a units
string and possibly even the base.