On Fri, Feb 7, 2014 at 8:55 AM, Christopher Sean Morrison <[email protected]> wrote: > 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? > 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. Hm, not too specific an intention but it all happened during my foray into the bitv world and its tests (I think I see room for refactoring and making the conversions a bit more user-friendly, error-tolerant, and robust). Among other things, the literals, whether global or not, need to be defined somewhere, and, IMHO, shouldn't be duplicated. I can certainly rename them in accordance with convention. >> +BU_EXPORT extern void bu_hexstr_to_binstr(const char *hexstr, struct bu_vls >> *b); > > Suggest making use of the return return code to WILCO >> +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. No argument there! Ideas welcome!. But I'm actually moving toward a consolidation of such conversions into a more general function--maybe the name would be too complicated now. It seems most of them can fit into two or three classes instead of so many individual ones that are variations on a theme. >> + 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). Hm, I didn't realize a stack bu_vls would be so much better since the guts use malloc. Any hoo, I've always thought a bu_vls tutorial would be useful. (and I did plan to use valgrind again...). > 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);) I'll clean it all up bu_vls-wise ASAP. Cheers (and thanks for the critique)! -Tom ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/brlcad-devel
