Hi Jeff, Thanks for the review.
> The docs still say "Control characters in the string will be replaced > with spaces", but they're being escaped now. That needs to be fixed. Done. > I note you overload the cast operator in your new class. Why not just > use an accessor? Was this style something someone else suggested? Yup. My C++ foo is very weak, so I asked Martin for help, and that was he suggested. Is this method wrong ? > Onward to the nits :-) :-) > + char *m_str; > + bool m_owned; > > I don't think we try to line up variable definitions like that. OK. Fixed. > + escaped = (char *) xmalloc (len * 2 + 1); > I believe that generally we want a slower growth pattern. You'll find > that we regularly do something like len * 3 / 2 + 1. Seems wise here too. Also done. > +void > +escaped_string::escape (const char * unescaped) > No need for the space between the * and unescaped. Et tu Jeff ? Then fall Nick. :-) [Fixed] > + case '\v': escaped[new_i++] = 'v'; break; > + default: escaped[new_i++] = '?'; break; > + } > I believe our coding standards would have the statements on new lines > rather than on the line with the case label. Sorted. > So I think the biggest question here is the cast overload vs just using > an accessor. OK, so demonstrating my ignorance: what is the accessor solution to this problem ? Cheers Nick PS. Revised patch attached in case the current solution of casting the operators is OK.
pr84195.patch.6
Description: Unix manual page
