Convert library -- Andrzej's review
Hi, This is the review of the Convert library. My vote is a CONDITIONAL YES: this library should be accepted into Boost subject to conditions described at the end of this message. This is a good and useful library. This is what I needed for a long while: a tool that allows me to convert a string to an int or float, but that would not treat the failure to convert as something unusual, and that would be locale-aware. Convert library offers this. It also offers other things that I fail to appreciate, but I accept that it is supposed to satisfy more expectations than just mine. Your choice of the generic type converter: bool operator()(TypeIn const&, TypeOut&); I think it is the right choice: this is the signature that does not impose any run-time overhead. Unlike boost::convert<TypeOut>::result, which does impose some run-time overhead of copying (or at least moving) the TypeOut. But that's acceptable. If the conversion needs to be super fast, one can fall back to using the converter directly. I appreciate the stringstream-based converter most. About the ability to plug other converters, I am just not convinced. Why would I use other converters? I know, lexical_cast is supposed to be faster in some cases, but if I was interested in this performance I would have used lexical_cast directly. The alleged ability to use this library to change the encoding of the strings or to convert between "related" types like ptime and time_t and Time -- I am not convinced: do you expect such conversions to fail and to report such failure the same way as you report the impossibility of interpreting a string as a float? I personally do not like this trick to force me to type this "from" in "convert<int>::from". If I was only passing my string as an argument, this would be fine, but when I am also passing the converter object, it reads that I am converting from both the converter and the string. But I do not intend to argue over it. You wrote the (good) library, so you have the privilege to pick the name. I am also not convinced about the potential of the 'facade' part. I can see that you can see the potential in it. But to me separating the library into the facade and the stringstream converter adds no practical value. But the stringstream converter itself is enough to stand for the usefulness of library. My condition on the acceptance of the library is that its performance should be improved as indicated by others, in particular, provide move semantics for boost::convert<TypeOut>::result (on compilers with rvalue references). It is enough that you commit to doing it in the future. Regards, &rzej
Andrzej Krzemienski <akrzemi1 <at> gmail.com> writes:
This is the review of the Convert library. My vote is a CONDITIONAL YES: this library should be accepted into Boost subject to conditions described at the end of this message. ...
Andrzej, thank you for your review and your "CONDITIONAL YES" vote. It's very much appreciated. As for "conditional", then that has always been my goal and expectation -- this current submission is far from being ready for inclusion as-is. If we decide to proceed down the path described in the submission, I'll certainly be extending std::sstream-based converter first (I use it most), etc.
I appreciate the stringstream-based converter most. About the ability to plug other converters, I am just not convinced. Why would I use other converters? I know, lexical_cast is supposed to be faster in some cases, but if I was interested in this performance I would have used lexical_cast directly.
Yes, I understand your reasoning. I personally find the pluggability quality to be quite important. Let me try and explain. First, from the first submission review it was clear to me that not everyone was interested in the std::sstream-based functionality and prepared to accept relative slowness of std::sstream. The pluggability addresses that issue with minimal effort and makes the library useful for much wider programming community. Secondly, the separation of the API/facade from the actual converter -- the pluggability -- allows to write and deploy new/better/domain-specific converters easily -- as long as those converters conform to the API-imposed contract. The difference in "plugging a new converter" vs "replacing an existing directly-used converter with a new converter" is considerable in a large-scale development as the former only replaces a pluggable component (leaving the "plumbing" intact) and the latter may create a lot of ripples in the related code. An every-day example might be unplugging and replacing an electrical device. Clearly, with no pluggability replacing such a device (directly connected to your house wiring) might be quite a hassle. Yes, on one-person-writing-new-code level that pluggability looks like a hassle. My argument is that on the large-code-base, maintenance, changed-requirements phase pluggability is the only sane way to manage that change.
The alleged ability to use this library to change the encoding of the strings or to convert between "related" types like ptime and time_t and Time -- I am not convinced: do you expect such conversions to fail and to report such failure the same way as you report the impossibility of interpreting a string as a float?
Well, I am not convinced either. :-) Still, I used it for MBCS to UCS string conversion in the past and did not see anything wrong with it. The main reason was to use an already familiar API instead of the need of inventing/learning/maintaining yet another new one.
I personally do not like this trick to force me to type this "from" in "convert<int>::from".
It's been a while and OTOH I do not remember what/how it was exactly but the main purpose of "from" is to separate TypeIn from TypeOut. Not template<class TypeOut, class TypeIn, class Converter> optional<TypeOut> convert(TypeIn, Converter) but template<class TypeOut> struct convert { static optional<TypeOut> from(TypeIn, Converter); } That way (at least I felt so back then) it was the only way to provide the ability to *reliably* specialize separately on TypeIn and/or TypeOut. Again, maybe that original decision/design is not valid or important and needs to be revisited.
If I was only passing my string as an argument, this would be fine, but when I am also passing the converter object, it reads that I am converting from both the converter and the string.
Yes, I remember you suggesting defaulting the converter parameter to the std::sstream-based and my initial intention to do just that. I then backed off as I realized the create-converter-once-use it-many-times can be easily lost. If we could come up with something like boost::convert::use(some_converter); int v = convert<int>::from(str); // some_converter used That might address your concern, right? Any crazy tricks to achieve that without inheritance?
...
I am also not convinced about the potential of the 'facade' part. I can see that you can see the potential in it. But to me separating the library into the facade and the stringstream converter adds no practical value. But the stringstream converter itself is enough to stand for the usefulness of library.
I tried addressing that somewhat above -- API facade is the consumer-provider contract. Based on that the consumer knows what to expect and the provider knows what to provide *without consumer-provider interaction*. In practical terms it is that every time I browse somebody else's code and see the interface, I immediately know what it does without reading the docs, learning new api, etc. In real life, when one is reading (trying to fix) somebody else's code the "reading the docs, learning new api" seems like a considerable diversion and often does not happen. So, the understanding of the code quickly turns into a guessing game. So, productivity is no more.
My condition on the acceptance of the library is that its performance should be improved as indicated by others, in particular, provide move semantics for boost::convert<TypeOut>::result (on compilers with rvalue references). It is enough that you commit to doing it in the future.
My intention, in fact, is to ditch convert<>::result for boost::optional given that you'll be pushing it into 1.56, right?
Vladimir Batov <vb.mail.247 <at> gmail.com> writes:
Andrzej Krzemienski <akrzemi1 <at> gmail.com> writes:
I personally do not like this trick to force me to type this "from" in "convert<int>::from".
It's been a while and OTOH I do not remember what/how it was exactly but the main purpose of "from" is to separate TypeIn from TypeOut. Not
template<class TypeOut, class TypeIn, class Converter> optional<TypeOut> convert(TypeIn, Converter)
but
template<class TypeOut> struct convert { static optional<TypeOut> from(TypeIn, Converter); }
That way (at least I felt so back then) it was the only way to provide the ability to *reliably* specialize separately on TypeIn and/or TypeOut. Again, maybe that original decision/design is not valid or important and needs to be revisited.
I just had another look and there seems no value anymore in potential specializations on TypeIn, TypeOut. It's because all the "smartness" and type-handling has been moved to converters. So, *there seems*, the "from" can be dropped. That is, int v = boost::convert<int>(str, cnv).value(); instead int v = boost::convert<int>::from(str, cnv).value(); and std::transform( strings.begin(), strings.end(), std::back_inserter(integers), convert<int, string>(ccnv(std::hex)).value_or(-1)); instead std::transform( strings.begin(), strings.end(), std::back_inserter(integers), convert<int>::from<string>(ccnv(std::hex)).value_or(-1)); I am on the fence (the "from" has been with me for some time). Any strong/weak :-) preferences, any for/against arguments?
On 05/18/2014 11:45 PM, Vladimir Batov wrote:
Vladimir Batov <vb.mail.247 <at> gmail.com> writes:
Andrzej Krzemienski <akrzemi1 <at> gmail.com> writes:
I personally do not like this trick to force me to type this "from" in "convert<int>::from". I just had another look and there seems no value anymore in potential specializations on TypeIn, TypeOut. It's because all the "smartness" and type-handling has been moved to converters. So, *there seems*, the "from" can be dropped. That is,
int v = boost::convert<int>(str, cnv).value();
instead
int v = boost::convert<int>::from(str, cnv).value();
and
std::transform( strings.begin(), strings.end(), std::back_inserter(integers), convert<int, string>(ccnv(std::hex)).value_or(-1));
instead
std::transform( strings.begin(), strings.end(), std::back_inserter(integers), convert<int>::from<string>(ccnv(std::hex)).value_or(-1));
I am on the fence (the "from" has been with me for some time). Any strong/weak :-) preferences, any for/against arguments?
Given a few people were not happy with boost::convert::from() interface I've added boost::cnv() for now (so that the names do not clash while both exist). int v = boost::cnv<int>(str, cnv).value(); does look cleaner and somewhat similar to lexical_cast deployment (for better or worse). However, I feel similarly confused about boost::cnv<int, string>(cnv).value_or(-1); as I always felt about lexical_cast when I had to specify both types boost::lexical_cast<int, string>("char string"); compared to more explicit convert<int>::from<string>(cnv).value_or(-1); I am easy about both ways and will go with whatever the majority settles on. Any feelings/preferences/arguments for one or the other?
On May 20, 2014 12:34:01 AM EDT, Vladimir Batov <Vladimir.Batov@constrainttec.com> wrote:
On 05/18/2014 11:45 PM, Vladimir Batov wrote:
Vladimir Batov <vb.mail.247 <at> gmail.com> writes:
Andrzej Krzemienski <akrzemi1 <at> gmail.com> writes:
I personally do not like this trick to force me to type this "from" in "convert<int>::from".
I don't mind the clear separation of the input and output types, but if "from" can be avoided easily, do so.
I just had another look and there seems no value anymore in potential specializations on TypeIn, TypeOut. It's because all the "smartness" and type-handling has been moved to converters. So, *there seems*, the "from" can be dropped. That is,
int v = boost::convert<int>(str, cnv).value();
instead
int v = boost::convert<int>::from(str, cnv).value();
and
std::transform( strings.begin(), strings.end(), std::back_inserter(integers), convert<int, string>(ccnv(std::hex)).value_or(-1));
instead
std::transform( strings.begin(), strings.end(), std::back_inserter(integers), convert<int>::from<string>(ccnv(std::hex)).value_or(-1));
I am on the fence (the "from" has been with me for some time). Any strong/weak :-) preferences, any for/against arguments?
Shorter is better when clarity is not lost, but I'm unclear on the need to specify the input type.
Given a few people were not happy with boost::convert::from() interface
I've added boost::cnv() for now (so that the names do not clash while both exist).
int v = boost::cnv<int>(str, cnv).value();
does look cleaner and somewhat similar to lexical_cast deployment (for better or worse). However, I feel similarly confused about
boost::cnv<int, string>(cnv).value_or(-1);
as I always felt about lexical_cast when I had to specify both types
boost::lexical_cast<int, string>("char string");
I haven't begun my review yet, though I intend to do so, so I might find the answer myself, but why is it necessary to specify "string" in that example? Can the converter be expected to understand a string literal as well as a std::string?
compared to more explicit
convert<int>::from<string>(cnv).value_or(-1);
The ordering of the two types is logical, and each is closely coupled with what it affects (return type and parameter type), but the "from" form leaves no room for misinterpretation. ___ Rob (Sent from my portable computation engine)
On 05/20/2014 07:15 PM, Rob Stewart wrote:
I don't mind the clear separation of the input and output types, but if "from" can be avoided easily, do so.
Encouraged ;-) I could not resist the itch and got rid of "::from". Updated the code and the docs.
Shorter is better when clarity is not lost, but I'm unclear on the need to specify the input type.
Indeed, it's not needed as often. Still, sometimes we might want to force a particular type: boost::cstringstream_converter cnv; boost::convert<string>(-1, cnv).value() == "-1"; boost::convert<string, unsigned int>(-1, cnv).value() == 4294967295; although one might argue for boost::convert<string>(uint(-1), cnv).value() == 4294967295; So, I agree, the need to specify the input type is mute right now.
boost::cnv<int, string>(cnv).value_or(-1); boost::lexical_cast<int, string>("char string"); I haven't begun my review yet, though I intend to do so, so I might find the answer myself, but why is it necessary to specify "string" in that example? Can the converter be expected to understand a string literal as well as a std::string?
Indeed, that was a bad example and the explicit specification of in-type was not necessary.
On 5/20/2014 9:29 PM, Vladimir Batov wrote:
On 05/20/2014 07:15 PM, Rob Stewart wrote:
I don't mind the clear separation of the input and output types, but if "from" can be avoided easily, do so.
Encouraged ;-) I could not resist the itch and got rid of "::from". Updated the code and the docs.
Please do not update the code or the docs while the review is ongoing. By doing so you make it impossible for everyone reviewing your source to be reviewing the same thing. You can of course make updates but not to the master branch.
Shorter is better when clarity is not lost, but I'm unclear on the need to specify the input type.
Indeed, it's not needed as often. Still, sometimes we might want to force a particular type:
boost::cstringstream_converter cnv;
boost::convert<string>(-1, cnv).value() == "-1"; boost::convert<string, unsigned int>(-1, cnv).value() == 4294967295;
although one might argue for
boost::convert<string>(uint(-1), cnv).value() == 4294967295;
So, I agree, the need to specify the input type is mute right now.
boost::cnv<int, string>(cnv).value_or(-1); boost::lexical_cast<int, string>("char string"); I haven't begun my review yet, though I intend to do so, so I might find the answer myself, but why is it necessary to specify "string" in that example? Can the converter be expected to understand a string literal as well as a std::string?
Indeed, that was a bad example and the explicit specification of in-type was not necessary.
On 05/21/2014 12:41 PM, Edward Diener wrote:
On 5/20/2014 9:29 PM, Vladimir Batov wrote:
On 05/20/2014 07:15 PM, Rob Stewart wrote:
I don't mind the clear separation of the input and output types, but if "from" can be avoided easily, do so.
Encouraged ;-) I could not resist the itch and got rid of "::from". Updated the code and the docs.
Please do not update the code or the docs while the review is ongoing. By doing so you make it impossible for everyone reviewing your source to be reviewing the same thing. You can of course make updates but not to the master branch.
Sorry, Edward. You are right. I am sorry. No more for the time being.
On 5/20/2014 10:50 PM, Vladimir Batov wrote:
On 05/21/2014 12:41 PM, Edward Diener wrote:
On 5/20/2014 9:29 PM, Vladimir Batov wrote:
On 05/20/2014 07:15 PM, Rob Stewart wrote:
I don't mind the clear separation of the input and output types, but if "from" can be avoided easily, do so.
Encouraged ;-) I could not resist the itch and got rid of "::from". Updated the code and the docs.
Please do not update the code or the docs while the review is ongoing. By doing so you make it impossible for everyone reviewing your source to be reviewing the same thing. You can of course make updates but not to the master branch.
Sorry, Edward. You are right. I am sorry. No more for the time being.
Please revert immediately all changes you have made to the master branch since the beginning of the review. You can save your current changes to another branch but you must return the master branch to the way it was when the review started. The review started on May 12 so you must revert your changes back to just before that date. It is possibly my fault but I thought that you knew that during a review no changes are allowed to the source being reviewed else people reviewing a library are seeing different source when they download the source for review.
On 05/21/2014 01:17 PM, Edward Diener wrote:
On 5/20/2014 10:50 PM, Vladimir Batov wrote:
On 05/21/2014 12:41 PM, Edward Diener wrote:
On 5/20/2014 9:29 PM, Vladimir Batov wrote:
On 05/20/2014 07:15 PM, Rob Stewart wrote:
I don't mind the clear separation of the input and output types, but if "from" can be avoided easily, do so.
Encouraged ;-) I could not resist the itch and got rid of "::from". Updated the code and the docs.
Please do not update the code or the docs while the review is ongoing. By doing so you make it impossible for everyone reviewing your source to be reviewing the same thing. You can of course make updates but not to the master branch.
Sorry, Edward. You are right. I am sorry. No more for the time being.
Please revert immediately all changes you have made to the master branch since the beginning of the review.
You can save your current changes to another branch but you must return the master branch to the way it was when the review started. The review started on May 12 so you must revert your changes back to just before that date.
It is possibly my fault but I thought that you knew that during a review no changes are allowed to the source being reviewed else people reviewing a library are seeing different source when they download the source for review.
Sorry, Edward, the fault is all mine. Now I remember you mentioning no-changes... just thought that late readers did not need to stumble over the issues mentioned by previous reviewers. I just branched/saved the latest changes to the "post_review" branch and I already have the "develop" branch which is in that "original" state. Now I am looking for how to reset "master" back to match "develop".
On 5/21/2014 12:25 AM, Vladimir Batov wrote:
On 05/21/2014 01:17 PM, Edward Diener wrote:
On 5/20/2014 10:50 PM, Vladimir Batov wrote:
On 05/21/2014 12:41 PM, Edward Diener wrote:
On 5/20/2014 9:29 PM, Vladimir Batov wrote:
On 05/20/2014 07:15 PM, Rob Stewart wrote:
I don't mind the clear separation of the input and output types, but if "from" can be avoided easily, do so.
Encouraged ;-) I could not resist the itch and got rid of "::from". Updated the code and the docs.
Please do not update the code or the docs while the review is ongoing. By doing so you make it impossible for everyone reviewing your source to be reviewing the same thing. You can of course make updates but not to the master branch.
Sorry, Edward. You are right. I am sorry. No more for the time being.
Please revert immediately all changes you have made to the master branch since the beginning of the review.
You can save your current changes to another branch but you must return the master branch to the way it was when the review started. The review started on May 12 so you must revert your changes back to just before that date.
It is possibly my fault but I thought that you knew that during a review no changes are allowed to the source being reviewed else people reviewing a library are seeing different source when they download the source for review.
Sorry, Edward, the fault is all mine. Now I remember you mentioning no-changes... just thought that late readers did not need to stumble over the issues mentioned by previous reviewers. I just branched/saved the latest changes to the "post_review" branch and I already have the "develop" branch which is in that "original" state. Now I am looking for how to reset "master" back to match "develop".
I think: git checkout master git reset --hard develop git push
On 05/22/2014 02:40 AM, Edward Diener wrote:
On 5/21/2014 12:25 AM, Vladimir Batov wrote:
On 05/21/2014 01:17 PM, Edward Diener wrote:
Please revert immediately all changes you have made to the master branch since the beginning of the review. ... ... how to reset "master" back to match "develop".
I think:
git checkout master git reset --hard develop git push
Thanks, Edward. Getting to know Git is overwhelming... I ended up literally deleting/removing 'master' and then branching new 'master' from 'develop'. Seems OK.
2014-05-20 6:34 GMT+02:00 Vladimir Batov <Vladimir.Batov@constrainttec.com>:
On 05/18/2014 11:45 PM, Vladimir Batov wrote:
Vladimir Batov <vb.mail.247 <at> gmail.com> writes:
Andrzej Krzemienski <akrzemi1 <at> gmail.com> writes:
I personally do not like this trick to force me to type this "from" in
"convert<int>::from".
I just had another look and there seems no value anymore in potential
specializations on TypeIn, TypeOut. It's because all the "smartness" and type-handling has been moved to converters. So, *there seems*, the "from" can be dropped. That is,
int v = boost::convert<int>(str, cnv).value();
instead
int v = boost::convert<int>::from(str, cnv).value();
and
std::transform( strings.begin(), strings.end(), std::back_inserter(integers), convert<int, string>(ccnv(std::hex)).value_or(-1));
instead
std::transform( strings.begin(), strings.end(), std::back_inserter(integers), convert<int>::from<string>(ccnv(std::hex)).value_or(-1));
I am on the fence (the "from" has been with me for some time). Any strong/weak :-) preferences, any for/against arguments?
Given a few people were not happy with boost::convert::from() interface I've added boost::cnv() for now (so that the names do not clash while both exist).
int v = boost::cnv<int>(str, cnv).value();
does look cleaner and somewhat similar to lexical_cast deployment (for better or worse). However, I feel similarly confused about
boost::cnv<int, string>(cnv).value_or(-1);
as I always felt about lexical_cast when I had to specify both types
boost::lexical_cast<int, string>("char string");
compared to more explicit
convert<int>::from<string>(cnv).value_or(-1);
I am easy about both ways and will go with whatever the majority settles on. Any feelings/preferences/arguments for one or the other?
A bike-shed issue, isn't it? Either choice has its flaws. I guess the best solution is that you make your chocie, and use it, unless someone is able to convince you otherwise. One question though, regarding your last example: convert<int>::from<string>(cnv).value_or(-1); this renders a "converter object" with the following signature: int operator()(std::string const&); // right? It is an equivalent of a "monomorphic" lambda. I guess you could invent another syntax, that does not require to specify this InType: convert<int>(cnv).value_or(-1); which would render a converter object with parametrized signature: template <typename InType> int operator()(const InType&); This would be an equivalent of a polymorphic lambda. This appears more useful, at the first sight. Any reason why you choose not to do so? Regards, &rzej
On 05/20/2014 09:58 PM, Andrzej Krzemienski wrote:
2014-05-20 6:34 GMT+02:00 Vladimir Batov <Vladimir.Batov@constrainttec.com>:
On 05/18/2014 11:45 PM, Vladimir Batov wrote:
Vladimir Batov <vb.mail.247 <at> gmail.com> writes:
Andrzej Krzemienski <akrzemi1 <at> gmail.com> writes:
I personally do not like this trick to force me to type this "from" in
"convert<int>::from". Given a few people were not happy with boost::convert::from() interface I've added boost::cnv() for now (so that the names do not clash while both exist).
int v = boost::cnv<int>(str, cnv).value(); A bike-shed issue, isn't it? Either choice has its flaws. I guess the best solution is that you make your chocie, and use it, unless someone is able to convince you otherwise.
Well, you felt strongly enough to mention that you did "not like this trick to force me to type this "from"", right? :-) Other people also mentioned it. So, I tried to adapt less objectionable interface.
One question though, regarding your last example:
convert<int>::from<string>(cnv).value_or(-1);
this renders a "converter object" with the following signature:
int operator()(std::string const&); // right?
That must be convert::from call with algorithms. With algorithms convert::from returns an "algorithm_helped" object which then calls the provided callable with the signature mentioned in the documentation.
It is an equivalent of a "monomorphic" lambda. I guess you could invent another syntax, that does not require to specify this InType:
convert<int>(cnv).value_or(-1);
which would render a converter object with parametrized signature:
template <typename InType> int operator()(const InType&);
I admit I have no slightest idea what "monomorphic lamda" is :-) but that is *exactly* what I've done while moving from convert::from() to simply convert(). For example, boost::array<int, 4> integers = {{ 15, 16, 17, 18 }}; std::vector<std::string> strings; boost::cstringstream_converter cnv; cnv(std::hex)(std::uppercase)(std::showbase); std::transform( integers.begin(), integers.end(), std::back_inserter(strings), *boost::convert<std::string>(cnv));* For now it has been moved to the post_review branch though. So far I only found boost::convert<std::string>(cnv)); has one limitation over boost::convert<std::string, int>(cnv)); Namely, the latter can do implicit conversions when the former does not. That is, for the former to work correctly I have to have a container of elements the *correct* type. With the latter I can feed to an algorithm a container of elements *convertible* to the explicitly specified TypeIn.
This would be an equivalent of a polymorphic lambda. This appears more useful, at the first sight. Any reason why you choose not to do so?
Do you think we could have a quick look at the post_review branch and let me know if that's what you had in mind?
2014-05-22 1:22 GMT+02:00 Vladimir Batov <Vladimir.Batov@constrainttec.com>:
I admit I have no slightest idea what "monomorphic lamda" is :-) but that is *exactly* what I've done while moving from convert::from() to simply convert(). For example,
boost::array<int, 4> integers = {{ 15, 16, 17, 18 }}; std::vector<std::string> strings; boost::cstringstream_converter cnv;
cnv(std::hex)(std::uppercase)(std::showbase);
std::transform( integers.begin(), integers.end(), std::back_inserter(strings), *boost::convert<std::string>(cnv));*
For now it has been moved to the post_review branch though.
So far I only found
boost::convert<std::string>(cnv));
has one limitation over
boost::convert<std::string, int>(cnv));
Namely, the latter can do implicit conversions when the former does not. That is, for the former to work correctly I have to have a container of elements the *correct* type. With the latter I can feed to an algorithm a container of elements *convertible* to the explicitly specified TypeIn.
This would be an equivalent of a polymorphic lambda. This appears more
useful, at the first sight. Any reason why you choose not to do so?
Do you think we could have a quick look at the post_review branch and let me know if that's what you had in mind?
I had a short look at post_review branch and it looks we are thinking about the same thing. My apologies for using posh names, I guess I am becoming a snob. Regarding the comparison between the two interfaces, I guess you could provide both (it is not a problem, is it?). First is good for a start. You do not have to think too much or type too much. Then if someone is interested in limiting the function overloads, she can learn how to use the second parameter. Regards, &rzej
On 05/22/2014 05:07 PM, Andrzej Krzemienski wrote:
Do you think we could have a quick look at the post_review branch and let me know if that's what you had in mind? I had a short look at post_review branch and it looks we are thinking about
2014-05-22 1:22 GMT+02:00 Vladimir Batov <Vladimir.Batov@constrainttec.com>: the same thing.
I am glad to hear that. I put it together in a hurry and was not sure I did not make some monumental blunder that I missed.
... Regarding the comparison between the two interfaces, I guess you could provide both (it is not a problem, is it?).
Unfortunately, from my experience it's not all roses as one might expect. It might be the case for an established library (although it complicates maintenance) but (I found it out the hard way) it is a disaster for a library trying to get approved. What happens is that there will be a group liking the interface #1 and there will be a group liking the interface #2. Then, the group #1 will criticize and attack the interface #2 and the other way around... and the library author stands in the middle of it all. It's quite hard as the author stands alone defending *his* decision to support both interfaces... as he gets attacked from both sides. Does not sound like much fun. ;-)
On May 21, 2014 7:22:23 PM EDT, Vladimir Batov <Vladimir.Batov@constrainttec.com> wrote:
On 05/20/2014 09:58 PM, Andrzej Krzemienski wrote:
2014-05-20 6:34 GMT+02:00 Vladimir Batov <Vladimir.Batov@constrainttec.com>:
On 05/18/2014 11:45 PM, Vladimir Batov wrote:
Vladimir Batov <vb.mail.247 <at> gmail.com> writes:
Andrzej Krzemienski <akrzemi1 <at> gmail.com> writes:
It is an equivalent of a "monomorphic" lambda. I guess you could invent another syntax, that does not require to specify this InType:
convert<int>(cnv).value_or(-1);
which would render a converter object with parametrized signature:
template <typename InType> int operator()(const InType&);
I admit I have no slightest idea what "monomorphic lamda" is :-)
A single argument your lambda. [snip]
So far I only found boost::convert<std::string>(cnv));
has one limitation over
boost::convert<std::string, int>(cnv));
Namely, the latter can do implicit conversions when the former does not. That is, for the former to work correctly I have to have a container of elements the *correct* type. With the latter I can feed to an algorithm a container of elements *convertible* to the explicitly specified TypeIn.
That's where a polymorphic lambda comes into play: template<class T> std::string operator()(T); That function call operator can accept various types, yet produce the expected result. ___ Rob (Sent from my portable computation engine)
2014-05-18 15:16 GMT+02:00 Vladimir Batov <vb.mail.247@gmail.com>:
Andrzej Krzemienski <akrzemi1 <at> gmail.com> writes:
This is the review of the Convert library. My vote is a CONDITIONAL YES: this library should be accepted into Boost subject to conditions described at the end of this message. ... If I was only passing my string as an argument, this would be fine, but when I am also passing the converter object, it reads that I am converting from both the converter and the string.
Yes, I remember you suggesting defaulting the converter parameter to the std::sstream-based and my initial intention to do just that. I then backed off as I realized the create-converter-once-use it-many-times can be easily lost. If we could come up with something like
boost::convert::use(some_converter);
int v = convert<int>::from(str); // some_converter used
That might address your concern, right? Any crazy tricks to achieve that without inheritance?
No; no globals, please. My remark is only about the choice of the name ("::from"): not the interface. In the other place you are suggesting to drop the "::from". This would satisfy my taste. The converter should be passed as the argument to the facade, or at least be the member of the state-full facade. Your choice is the right one.
My condition on the acceptance of the library is that its performance should be improved as indicated by others, in particular, provide move semantics for boost::convert<TypeOut>::result (on compilers with rvalue references). It is enough that you commit to doing it in the future.
My intention, in fact, is to ditch convert<>::result for boost::optional given that you'll be pushing it into 1.56, right?
Well, at this point there are chances that all the necessary changes to Boost.Optional will not make their way to 1.56.0 release. Currently move semantics are ready, but function value() and value_or() are still missing and my velocity has shrunk since my daughter was born. It is also not certain when 1.56 is scheduled to be released. So I recommend that for 1.56 you have some plan B: either stick to your own type, or accept Boost.Optional with the limited (for the time being) interface. Regards, &rzej
I forgot to answer the standard questions, so here they are: What is your evaluation of the design?
It is good: the converter concept has been adequately identified, considering performance. The responsibilities (the conversion itself an the usage conveniences) have been correctly separated. What is your evaluation of the implementation?
It is satisfactory. What is your evaluation of the documentation?
I must admit that had it not been for the email exchanges and discussions here in the group, I would have troubles figuring out what the library is capable of from the documentation. I mean, every piece of information is there, but the focus on the comparisons with lexical_cast is distracting. The first thing I want to know about the conversion library is: is it a ware of different locales and if I can easily (without performance implications) check the conversion failure. I would expect the initial page to be short(er) and give answers to my questions. Apart form the above remark, I believe that the documentation contains every piece of information that is needed. What is your evaluation of the potential usefulness of the library?
Very useful! There is no standard satisfactory way of converting a string to an int/float in C++ community.
Did you try to use the library? With what compiler? Did you have any problems?
I played with the pre-review versions on VC10. Basic examples worked fine. How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
I studied the documentation of this and the previous versions. Looked at the implementation of the facade of pre-review versions and had a short look at the current. Are you knowledgeable about the problem domain?
I often need to convert, and had occasionally need to write my own simple conversion functions Regards, &rzej 2014-05-17 22:33 GMT+02:00 Andrzej Krzemienski <akrzemi1@gmail.com>:
Hi, This is the review of the Convert library. My vote is a CONDITIONAL YES: this library should be accepted into Boost subject to conditions described at the end of this message.
This is a good and useful library. This is what I needed for a long while: a tool that allows me to convert a string to an int or float, but that would not treat the failure to convert as something unusual, and that would be locale-aware. Convert library offers this. It also offers other things that I fail to appreciate, but I accept that it is supposed to satisfy more expectations than just mine.
Your choice of the generic type converter: bool operator()(TypeIn const&, TypeOut&); I think it is the right choice: this is the signature that does not impose any run-time overhead. Unlike boost::convert<TypeOut>::result, which does impose some run-time overhead of copying (or at least moving) the TypeOut. But that's acceptable. If the conversion needs to be super fast, one can fall back to using the converter directly.
I appreciate the stringstream-based converter most. About the ability to plug other converters, I am just not convinced. Why would I use other converters? I know, lexical_cast is supposed to be faster in some cases, but if I was interested in this performance I would have used lexical_cast directly. The alleged ability to use this library to change the encoding of the strings or to convert between "related" types like ptime and time_t and Time -- I am not convinced: do you expect such conversions to fail and to report such failure the same way as you report the impossibility of interpreting a string as a float?
I personally do not like this trick to force me to type this "from" in "convert<int>::from". If I was only passing my string as an argument, this would be fine, but when I am also passing the converter object, it reads that I am converting from both the converter and the string. But I do not intend to argue over it. You wrote the (good) library, so you have the privilege to pick the name.
I am also not convinced about the potential of the 'facade' part. I can see that you can see the potential in it. But to me separating the library into the facade and the stringstream converter adds no practical value. But the stringstream converter itself is enough to stand for the usefulness of library.
My condition on the acceptance of the library is that its performance should be improved as indicated by others, in particular, provide move semantics for boost::convert<TypeOut>::result (on compilers with rvalue references). It is enough that you commit to doing it in the future.
Regards, &rzej
participants (5)
-
Andrzej Krzemienski -
Edward Diener -
Rob Stewart -
Vladimir Batov -
Vladimir Batov