Tom,
Few thoughts and feedback on the recent addition below. In all, looks good but
where is this to be used? Do you already have something in mind?
On Feb 6, 2014, at 6:15 PM, tbrowd...@users.sourceforge.net wrote:
> Modified: brlcad/trunk/include/bu.h
> ===================================================================
> --- brlcad/trunk/include/bu.h 2014-02-06 22:53:22 UTC (rev 59738)
> +++ brlcad/trunk/include/bu.h 2014-02-06 23:15:47 UTC (rev 59739)
> @@ -1396,6 +1396,15 @@
> */
>
> /**
> + * some handy literals for bit twiddling
> + */
> +static const unsigned BITS_PER_BYTE = 8;
> +static const unsigned BITS_PER_HEXCHAR = 4;
> +static const unsigned HEXCHARS_PER_BYTE = 2;
> +static const unsigned HEX_BASE = 16;
> +static const unsigned BINARY_BASE = 2;
As an API, these are four undocumented new symbols and all are inconsistent
with the predominant prefix convention. Granted, that'd just make them longer
but I'm dubious of their need to be part of the API.
A calling application wouldn't actually be using them, would they? Seems
that's the point of the functions, to make the conversion to/from string all
just happen.
> +/**
> + * Convert a string of hex characters to an equivalent string of
> + * binary characters.
> + *
> + * The input hex string may have a prefix of '0x' or '0X' in which
> + * case the resulting binary string will be prefixed with '0b'.
> + *
> + * The input string is expected to represent an integral number of
> + * bytes but will have leading zeroes appended as necessary to fulfill
> + * that requirement.
> + *
> + */
> +BU_EXPORT extern void bu_hexstr_to_binstr(const char *hexstr, struct bu_vls
> *b);
Suggest making use of the return return code to indicate the error conditions
you detected in the implementation. Either a code or (probably better) a const
char * (from the bu_vls). What happens (docs should say) if either pointer is
NULL?
> +/**
> + * Convert a string of binary characters to an equivalent string of
> + * hex characters.
> + *
> + * The input binary string may have a prefix of '0b' or '0B' in which
> + * case the resulting hex string will be prefixed with '0x'.
> + *
> + * The input string is expected to represent an integral number of
> + * bytes but will have leading zeroes appended as necessary to fulfill
> + * that requirement.
> + *
> + */
> +BU_EXPORT extern void bu_binstr_to_hexstr(const char *binstr, struct bu_vls
> *h);
Same as bu_hexstr_to_binstr comments, but the naming convention seems
cumbersome. Just thinking out loud right now. I'm not sure I have a better
suggestion, but it seems difficult to make this fit our bu_NOUN_VERB pattern
without treating binstr and hexstr as new groups altogether. Thoughts?
> +void
> +bu_binstr_to_hexstr(const char *bstr, struct bu_vls *h)
> +{
> + struct bu_vls *b = bu_vls_vlsinit();
> + struct bu_vls *tmp = bu_vls_vlsinit();
Curious, particular reason these are not on the stack? Not that performance is
the issue, but it's much faster initialization and (more importantly) simpler,
less error prone (see below).
> + bu_log("WARNING: Incoming string length (%zu) not an integral number
> of bytes,\n", len);
> + bu_log(" padding with leading zeroes to ensure such.\n");
A NULL parameter will result in this message, misleading. Didn't check if bstr
or h parameters on entry.
> + bu_vls_trunc(h, 0);
App will bomb here if h is NULL.
> + bu_vls_free(b);
> + bu_vls_free(tmp);
> +
> +}
Memory leak here because bu_vls_vls_init() must be paired with
bu_vls_vls_free() or a separate bu_free(). Avoided if you put them on the
stack (e.g., struct bu_vls tmp = BU_VLS_INIT_ZERO; ... bu_vls_free(&tmp);)
Cheers!
Sean
------------------------------------------------------------------------------
Managing the Performance of Cloud-Based Applications
Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
Read the Whitepaper.
http://pubads.g.doubleclick.net/gampad/clk?id=121051231&iu=/4140/ostg.clktrk
_______________________________________________
BRL-CAD Developer mailing list
brlcad-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/brlcad-devel
