Re: [boost] [review] string convert

Subject: Re: [boost] [review] string convert
From: Gordon Woodhull (gordon_at_[hidden])
Date: 2011-05-03 01:13:39

Hi Vladimir, all,

First off, I'll respond to Rob and Jeremy.

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);

Well, Jeremy pointed out

> You propose that a bunch of tag_ names be used, but in practice they would likely have to be something like boost::convert::tag_, which would substantially reduce the benefits of any such change.

So, yes, I guess dont_throw or any tag type put into the template parameters, is basically disqualified by its verbosity.  That's too bad, because it would be nice to have a single function that returned either optional or a value.  

I suppose that Vladimir's nice named parameters exhibit the same problem.  Yuck.  Nonetheless, my objections and simplifications are quite orthogonal.

On May 2, 2011, at 10:35 PM, Vladimir Batov wrote:
> If the library is accepted (which seem unlikely so far and that's truly OK with
> me), then I am prepared to discuss and try out alternative approaches (if they
> are in some concrete "try-able" form).

Yes this is the review deadlock/stalemate problem I referred to earlier.  Anyway, I remain in favor of the library with simplified semantics.  It only boils down to three things I find distressing.

1. A special context which causes an apparent function call to either throw or not throw.
2. A streaming operation outside an apparent function call which applies IO manipulators within it.
3. "::from"

I'll drop #4, the multiple operator(), because that's common practice and it's quite clear what that's doing.

> The reason I've "come up" with the API as
> it is now is because during discussions some 2-3 years ago I in fact tried
> various proposed interfaces as we were discussing them. The results of those
> trial-n-errors is this current API.

I apologize that I still haven't reread those.  From what I remember, these issues were raised but I don't remember whether there was consensus how to fix them.

Despite that this review might return a negative result, I think it's been a valuable discussion.  

>> 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.

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&);
}
"

> I'd try distinguishing "not simple enough" and "annoying". The *user* API is
> one (!) function -- convert::from -- on one side and *optionally*
> convert::result on the receiving side. I really do not know how to simplify it
> any further. I personally do not consider changing that "annoying" from() to
> something else to be a simplification. So, by "simplification" you probably had
> something else in mind and I am all prepared to listen.

The complexity I and others are referring to is obviously not the number of functions!  It's the fact that the converter<> object returned by from() has at least three different behaviors, when I think most people from looking at it would think conversion has already happened and a simple value is being returned.

- It can throw if assigned to a regular value
- It doesn't throw if assigned to ::result
- It can be a functor which takes a string and returns a value, if the string was omitted in the constructor (?) or if the string was empty (?)

> As for "I don't like "::from" at all", then it's a personal choice and we'll never get a consensus here.

True.

> 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

> 2) the syntax has to be 
> convert<TypeOut>::from<TypeIn> (replace 'convert' and 'from' with the
> identifiers of your choice), i.e. two types need to be evaluated separately.
> Otherwise, various specializations (if there are such) conflict. 
> That's why the lexical_cast-like API (where both TypeIn and TypeOut evaluated at
> the same time) does not work (at least I did not manage it to work).

Again, see Vicente's library for the solution.

>> ... seem to require this weird static member syntax. 
> 
> I am not sure I am following. What's exactly weird about it (apart from your
> personal syntax preferences)? It's the standard C++ syntax, right?

I guess I'd expect something "more" to happen, like a side effect of instantiating the convert<> template, or having some value to instantiating a convert<> object or something.  Instead it's really just a function call where some of the template parameters are pushed into a class.

At least now I understand why you're doing this, although it appears to be unnecessary.

>> So, next '::result'. It's some form of optional return.
>> So why not just return optional?  It is not much less
>> to type
>> convert<int>::result maybe = convert<int>::from("not an int");
>> versus
>> optional<int> maybe = convert_cast<int, use_opt>("not an int");
> 
> Yes, convert::result does not offer *much* beyond boost::optional. However, it
> *does* offer more. That's the reason it exists. Namely, convert::result provides
> two pieces of information -- if the conversion failed/succeeded *and* the
> fallback value (if provided). boost::optional provides one *or* the other. The
> difference might be considered subtle but it's there.  

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.  

If that's really wanted, I think simply pair<bool,T> would be more clear.

>> Why should the LHS modify the behavior of the proxy object returned by ::from?
> 
> Because usages of 'convert' differ -- namely, throwing and non-throwing
> behavior. I personally do not use throwing behavior at all but I feel it was
> needed to provide/support the behavior (and resulting mess) for lexical_cast
> backward compatibility and to cater for usages different from mine.

Personally I would want both behaviors.  It's just the context-sensitive value that I object to.

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);

> 
>> It's far too much to think about for such a simple operation.  
> 
> Not really. Well, I do not see it that way anyway.
> 
> 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).  

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.

> The huge *practical* advantage of operator() is chaining. I personally do not
> see one or the other of the below simpler or harder and both are of equal
> length:
> 
>   from(s, _manip = std::hex);
>   from(s)(_manip = std::hex);
> 
> However, the first one has the limit on the number of args. Second does not. 
> Second is much easier/cleaner to implement (therefore, to maintain which is even
> more important) as #1 requires from() overloading or def. parameters. Thirdly,
> second looks more natural as a progression of increasing complexity as the user
> gains additional knowledge:
> 
>   int i = from(s); // simplest
>   int i = from(s)(_manip = std::hex);
>   int i = from(s)(_manip = std::hex)(locale_ = ...);

Yeah, I get this now.  Thanks!

>> On a similar nice-feature-but-weird-syntax note, let's look at the Using
>> Boost.Convert with Standard Algorithms section, which offers a few ways that
>> you can use convert<T>::from() to create a functor:
>> 
>> std::transform(
>>    strings.begin(),
>>    strings.end(),
>>    std::back_inserter(integers),
>>    convert<int>::from(string(), -1) >> std::hex);
>> ...
>> I think it would be much more appropriate to provide a Phoenix function object
>> for the purpose, something like 
>> std::transform(
>>    strings.begin(),
>>    strings.end(),
>>    std::back_inserter(integers),
>>    convert_<int>(_1, -1, std::hex));
>> Sigh of relief.  I see what that means immediately, ...
> 
> Apologies for sounding as a broken record but again that preference seems quite
> personal. As a user not familiar with Phoenix your variant would not look
> familiar to me. In order to use it I'd need to learn/remember/know additional
> API -- convert_, _1, args order. My deployment seems simpler (not
> surprisingly some bias here :-) ) as the user uses the same API and does not
> need to learn more to use 'convert' with algorithms.
> 
> Mentioning that not for the sake of arguing tooth and nail for *my* API. I am 
> happy to consider alternatives. I just do not see your suggested API as a clear
> winner (not yet anyway).

I think Boost.Lambda/Boost.Phoenix is pretty well-known syntax by now, but perhaps I've been indoctrinated.

I guess your syntax is a bit like currying - drop an argument and it becomes a function object which takes that argument.  Don't think I've seen that in C++ before.

> 
>> I also want to mention type-to-type conversion, which Vicente Botet has
>> thought out much more thoroughly in his Conversion library. This library
>> offers type-to-type conversion seemingly as an afterthought, just because the
>> syntax allows it.  
> 
> I have to disagree. Again I consider 'convert' to be more of a conversion
> framework rather than a library. Indeed, 'convert' does not provide much more
> beyond string-to-type conversions because I personally did/do not need more.
> However, it does not mean I/we should deny that possibility to others as their
> needs may differ. Therefore, 'convert' was designed with that in mind. If
> Vicente needs type-to-type conversion functionality, I think he might consider
> implementing that functionality within the 'convert' framework by extending the
> 'convert' library for everyone's benefit.

Leaving aside which is a more general framework... I believe yours falls back to type-to-string-to-type whereas his does not.  Both are valid behaviors.

> I do not think just one person can provide every possible conversion. I cannot anyway. And with 'convert' I am not saying I do.

Extensibility is essential.

> I believe I did implement the functionality
> that I immediately needed. More so, I tried to address concerns/suggestions
> brought up while the library was being written. So, I feel it might be a good
> basis from which we could move *forward* improving it -- adding conversions
> others needed, optimizing existing conversions.

I do think it's a good start.

> Maybe an alternative might be to delay reviewing this library for a
> year (?) and let guys try extending lexical_cast as there were numerous
> suggestions. Then, we'll re-evaluate the situation. Although my suspicion is
> that in 1-2 years that lexical_cast-extension push will fizzle out once again,
> new people will come in asking the same questions and we won't move an inch.

I think I made suggestions which answer almost all of the concerns raised in this review, except for the insistence on lexical_cast.

The functionality is clearly there.  It's just this odd use of Expression Templates where IMO it's not needed, that irks me.  
For what?  
To specify a context where a function won't throw.
To apply io manipulators using a familiar operator.
As a shortcut for lambdas.

Thanks for explaining all of this - I feel a lot better knowing why the library is how it is, especially that peculiar ::from.  I still disagree and hope you'll consider the "simplifications" I've suggested.  (I doubt they are original in any way!)  

If not, there'll probably be another lexical_cast debate soon enough.

Cheers,
Gordon