On 05/03/2018 03:55 AM, Nick Clifton wrote:
> 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 ?
As is so often the case, there's more than one way to do everything.
Overloading operators is allowed, but it's generally not my first choice.
>
>> 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 ?
So an accessor is just a member function to retrieve the data. You
could embed the cast there to convert. So instead of:
+ operator const char *() const { return (const char *) m_str; }
You have something like:
const char *get (void) { return (const char *) m_str; }
And instead of casting at the use site, you just call the get() method.
I think your overloaded cast is simple and unsurprising (which are
essentially the requirements for using an operator overload in our
conventions). So I can live with the overload.
For reference:
https://gcc.gnu.org/codingconventions.html#Over_Oper
>
> PS. Revised patch attached in case the current solution of casting the
> operators is OK.
OK. Please install.
Thanks,
jeff
