Re: [boost] [review] string convert

Subject: Re: [boost] [review] string convert
From: Jeff Flinn (TriumphSprint2000_at_[hidden])
Date: 2011-05-03 11:15:37

Stewart, Robert wrote:
> Gordon Woodhull wrote:
>> Here's again the alternate "simpler" syntax which I put in my
>> original review, which I think must have gotten buried from
>> typing too much.  :-/
>>
>> optional<int> i = string_convert<int, dont_throw>(s);  // or
>> string_convert<int>(s, _throw=false) if that's impossible
>> int i = string_convert<int>(s, manip_ = std::hex);
> 
> That's only a portion of the functionality of the library under review.
> 
>>>> It is incredibly clear what lexical_cast does because it
>>>> uses the standard meaning of the operators, function
>>>> calls, and return values.
>>> int i = lexical_cast<int>(str);
>>> int i = convert<int>::from(str);
>>>
>>> When I look at the above where lexical_cast and 'convert'
>>> provide the same functionality, I am not sure if from the
>>> user perspectives I immediately see how 'convert' differs
>>> from lexical_cast complexity-wise. I am certainly biased.
>> It's two more tokens, and I don't see the value to the user of
>> separating the two parameters.  I admit it's partly a matter of
>> taste, but I think following the lexical_cast syntax as far as
>> possible has objective value.
> 
> Using the new-style cast interface is certainly good.  some_cast<int>(str) can actually call convert<int>::from(str), so the other aspects of the library can be retained (in whatever form may settle out of this review) without harm.  Finding the right prefix for "_cast" will be a challenge.
> 
>> Since you say it's there for technical reasons, let's take a
>> look at how Vicente fixed it in his Conversion library.  I
>> think this is the relevant section of his doc:
>>
>> "
>> How to specialize the conversion functions
>> You can add an overload of convert_to and assign_to functions
>> as you will do for the swap function for example, but we have
>> to use a trick to allow ADL and avoid infinite recursion and
>> ambiguity. This trick consists in adding a unused parameter
>> representing
>> the target type. E.g. if you want to add an explicit conversion
>> from a type A to a type B do the following:
>> namespace my_own_namespace {
>>     B convert_to(const A& from, boost::dummy::type_tag<B>
>> const&);
>> }
>> "
> 
> Don't forget the other two use cases the library supports: no exception on failure (for types with and without sentinel values) and function objects.
> 
>>> The practical reason for "::from" is two-fold --
>>> 1) it clearly shows the direction of the conversion (which's
>>> been deemed important during original discussions);
>> Again, I haven't reread those discussions.  I am surprised that
>> people would find a function call confusing.  Usually it takes
>> its input as an argument and returns its result.  :-p
> 
> One case that occurs to me is because of the need to specify both target and source types in some cases:
> 
>    int const i(convert<int,Foo>("example"));
> 
> Obviously, that can be written like this instead:
> 
>    int const i(convert<int>(Foo("example")));
> 
> However, int a generic context, that may not be appropriate.  What if Foo is noncopyable?  Is convert<>()'s argument passed by value or reference?
> 
> Maybe I'm just trying to create a problem, but there was certainly some context that led to that concern.
> 
>> So the intention is something like this?
>>
>> convert<int>::result i = convert<int>::from(s, 17);
>> if(!i)
>>   return i.value();  // returns 17 (!)
>>
>> That looks pretty weird to me.  I don't see why someone would
>> want to specify the default value if they are then going to
>> check if conversion succeeded.
> 
> Did you read the documentation?  There's an example for this use case.  The issue arises when a type has no appropriate sentinel value to indicate the failure and is noncopyable.  In that case, one must construct an instance with a real initial state.  That instance can be passed as the fallback value (17, above).  Getting the fallback value from the result object isn't proof that the conversion failed in that case.  Instead, one must query the result object and, if the conversion succeeded, retrieve the value.
> 
>> If that's really wanted, I think simply pair<bool,T> would be
>> more clear.
> 
> That imposes Copyable on T.
> 
>> Is the following syntax possible?  I am not up on
>> Boost.Parameter yet.
>>
>> int i = convert<int>::from(s)(_dothrow);
>> optional<int> j = convert<int>::from(s)(_dontthrow);
> 
> Aren't you asking for the very thing you've been against?  convert<int>::from() returns two different things depending upon context, right?  I'd also argue against requiring _dothrow and _dontthrow.  One should be the default.
> 
>>> convert<int>::result res = convert<int>::from("blah", -1);
>>> if (!res) message("conversion failed");
>>> int value = res.value(); // proceed with the fallback anyway.
>>>
>>> The locality of 'convert' application (IMHO of course) does
>>> not leave much to think how 'convert' is to behave.
>> Clear enough in itself (although I'm still reeling from the
>> idea of an object which converts to false having a value).
> 
> For non-DefaultConstructible types.
> 
>> It's just that the next line might read
>>
>> int res2 = convert<int>::from("blah");
>>
>> and throw, and there's no indication why one version throws and
>> the other does not.
> 
> Of course there is.  One was given a fallback value and the other wasn't.

dynamic_cast sets a precedent for how to handle throwing/non-throw 
variations:

   dynamic_cast<X&>(y); // throws if cast fails

   X* x = dynamic_cast<X*>(y); // returns 0

So if boost::optional is the analog of pointer, and we go to some 
xxx_cast naming convention we could have:

   // throws if fails
   int i = xxx_cast<int>(s);

   // returns empty optional if cast fails
   boost::optional<int> oi = xxx_cast<boost::optional<int>>(s);

   // never fails
   int i = xxx_cast<int>(s, 123);

This just reuses boost::optional rather than the (superfluous to me) 
convert<T>::result type. I for one see no reason that _cast need be 
limited to a single argument. If others find multiple arguments too 
disgusting, perhaps boost::make<int>(s, 123) is more palatable.

Jeff