Unsigned integers aren't parsed correctly with mingw

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Emit an unsigned int scalar.  (A cast to int is necessary).
2. Parse said scalar - an exception is thrown.
3.

MinGW 5.1.6.

I traced into the code at conversion.h.  Several problems I noticed and
fixed, please consider the following as a patch.

1. Checking the stringstream for fail causes Convert to return false with
MinGW (but not VS!).  If you use the result of the extraction, you get a
result for what you really care about anyways - we don't care if the
stringstream has troubles now, we only care if the data was extracted.

2. I got tired of casting unsigned ints on Emission and hoping that things
would work on parsing.  I got rid of the macros and put in a template
version that should include all of the unsigned integral types along with
64 bit types (which I also needed).  =)

--------------- Replace macros & such in conversion.h ------------
    // This is your generic converter, it will do all integral types, etc.

    // It should work with 64 bit integers too.

    template< typename T >

    inline bool Convert(const std::string& input, T& output) {

        std::stringstream stream;

        stream << input;

        stream.unsetf( std::ios::dec );  // The stream will now convert out
decimal, hex and even octal integers.

        return stream >> output;    // Don't use fail, it doesn't do what
you think it does.

                                    // you really only care about the
result of _this_ extraction.

    }

    // Specializations.

    template<>

    inline bool Convert(const std::string& input, std::string& output) {

        output = input;

        return true;

    }

    // The non template functions that get forwarded too.

    bool ConvertBool_( const std::string& input, bool& output );

    bool ConvertNull_( const std::string& input, _Null& output );

    // Forward this on to the non template function that does some boolean
magic.

    template<>

    inline bool Convert(const std::string& input, bool& output) {

        return ConvertBool_(input, output);

    }

    template<>

    inline bool Convert(const std::string& input, _Null& output) {

        return ConvertNull_(input, output);

    }   

----- conversion.cpp ------------
Just rename the two free funcs:

bool ConvertBool_(const std::string& input, bool& b)

bool ConvertNull_( const std::string& input, _Null& output );

Let me know if you have any questions, I'm happy to help.

Original issue reported on code.google.com by tim.fi...@gmail.com on 6 Nov 2009 at 2:34

[GoogleCodeExporter]

Thanks. I committed two changes (the latest is r321) which do the following:

1. Switched to returning the result of `operator >>` for the conversion, and 
unset 
the dec flag.

2. Overloaded a bunch of integral types for the emitter.

We can't have an unqualified template class Convert, because if you overloaded 
`operator >>` for your own type `Foo` and then tried to use it as a key in a 
map, it 
would try `Convert<Foo>` and fail before it found your overloaded `operator >>`.

If you have more types you'd like me to overload (the macro seems like the best 
option for this, but if you have a better idea that's not just an unqualified 
template, I'd be happy to hear it), let me know (and if they're not portable 
types, 
just tell me what #ifdefs to use).

I have a similar feeling for `Emitter::Write`. I overloaded a bunch more 
integral 
types (so you can output unsigned ints, for example), and if you have any more 
suggestions, I'd be happy to hear them.

Thanks for all your support!

Original comment by jbe...@gmail.com on 6 Nov 2009 at 3:30

• Changed state: Started

[GoogleCodeExporter]

I'm not sure I understand the part about operator >> and Convert on custom 
types.  Is
there a good reason to use a custom type for a key?  It had never occurred to 
me to
do so, so I never thought of the matching problem.

Yeah, a better way than macros might be to use an integral typelist, to catch 
all
integrals into a single template function.

Original comment by tim.fi...@gmail.com on 6 Nov 2009 at 5:45

[GoogleCodeExporter]

YAML allows arbitrary nodes to be keys for maps. For a simple example, suppose 
we
have a YAML doc that looks like:

    the grid:
      [0, 1]: dragon
      [1, -1]: ogre
      [2, 0]: wizard

It would be nice if we could say

    struct Pt {
       Pt(int x_, int y_): x(x_), y(y_) {}
       int x, y;
    };
    // ...
    const YAML::Node& grid = doc["the grid"];
    std::string character_type;
    grid[Pt(0, 1)] >> character_type;

To do this (as the library is now), you just need to define

    bool operator == (const Pt& v, const Pt& w) {
       return v.x == w.x && v.y == w.y;
    }

    void operator >> (const YAML::Node& node, Pt& p) {
       p.x >> node[0];
       p.y >> node[1];
    }

and it'll work fine. The reason is magic in nodereadimpl.h. The idea is, if

    Convert(std::string, T)

exists, then call it; otherwise, call `operator >> (const Node&, T&)`. But the 
lookup
for whether `Convert` exists is in the first scan of the compiler, before it 
would
try to instantiate the templated `Convert`. So even if `Convert<Pt>` would fail 
to
compile (since there's no

    operator >> (std::stringstream&, Pt&)

defined), the compiler still sees that the *declaration* for it exists, so it 
tries
to call it. But then it fails during the second pass, when we actually try to
instantiate it.

Basically, the end of the story is that we can't have a templated Convert 
function,
since it would disrupt any user defined keys; we have to overload Convert 
instead.

Original comment by jbe...@gmail.com on 6 Nov 2009 at 9:41

[GoogleCodeExporter]

Ah!  That makes sense, thanks for the explanation.  

I'll try out your fixes later.  I have something working for my specific 
purposes for
now.

Original comment by tim.fi...@gmail.com on 6 Nov 2009 at 9:52

[GoogleCodeExporter]

I figured that typelists might be of a help here.  Turns out there's a
metaprogramming idiom that fits this problem exactly (function overloading 
signature
matching).

http://en.wikipedia.org/wiki/Substitution_failure_is_not_an_error

Here's a version that uses it:

// The default template instantiation is false.
template < typename >
struct IsFundamental { enum { value = false }; };

template <> struct IsFundamental< char > { enum { value = true }; };
template <> struct IsFundamental< const char* > { enum { value = true }; };
template <> struct IsFundamental< char* > { enum { value = true }; };

template <> struct IsFundamental< unsigned char > { enum { value = true }; };

template <> struct IsFundamental< int > { enum { value = true }; };
template <> struct IsFundamental< unsigned int > { enum { value = true }; };
template <> struct IsFundamental< long int > { enum { value = true }; };
template <> struct IsFundamental< unsigned long int > { enum { value = true }; 
};
template <> struct IsFundamental< short int > { enum { value = true }; };
template <> struct IsFundamental< unsigned short int > { enum { value = true }; 
};
template <> struct IsFundamental< long long > { enum { value = true }; };
template <> struct IsFundamental< unsigned long long > { enum { value = true }; 
};

template <> struct IsFundamental< float > { enum { value = true }; };
template <> struct IsFundamental< double > { enum { value = true }; };
template <> struct IsFundamental< long double > { enum { value = true }; };

template < typename T, typename, bool = T::value > 
struct Enable_if;

template < typename T, typename R >
struct Enable_if <T, R, true> {
    typedef R type;
};

template <typename T> 
inline bool Convert(string& s, T t, typename Enable_if< IsFundamental<T>, T 
>::type*
dummy = 0) {
    std::stringstream stream;
    stream.unsetf(std::ios::dec);
    return stream << t && stream >> s;
}

Original comment by tim.fi...@gmail.com on 9 Nov 2009 at 5:04

[GoogleCodeExporter]

Good idea. I've committed a patch (r322) that uses essentially this idea. I 
believe
this is the end of the issue, but if you have any other related comments, 
please let
me know.

Thanks!

Original comment by jbe...@gmail.com on 10 Nov 2009 at 9:25

• Changed state: Fixed

[GoogleCodeExporter]

Hi guys, yes I also considered the SFINAE solution.  Again I must recommend a 
boost
library for this, as there already is a clean implementation of this.

Take a look at boost::enable_if.  It allows for conditional removal of template
overloads.

Cheers,

Carter

http://www.boost.org/doc/libs/release/libs/utility/enable_if.html

Original comment by ctvo...@gmail.com on 11 Nov 2009 at 5:19

[GoogleCodeExporter]

Hi Carter,

boost::enable_if is essentially what we've done here. It's a solid two lines of 
code :)

Original comment by jbe...@gmail.com on 11 Nov 2009 at 6:03

[GoogleCodeExporter]

Yes, I saw the boost enable_if too, very nice.  What's the reason for not using
boost?  Dependencies?  

Original comment by tim.fi...@gmail.com on 11 Nov 2009 at 6:09

[GoogleCodeExporter]

Tim,

Yes, more or less. For things like boost::enable_if, there's no reason to add a
dependency for just a couple lines of code. For things like boost::regex, it's
appealing, but I'm inclined to avoid dependencies if I can. Not everyone has 
boost
installed, and while there are certainly easy-ish ways of setting up the
dependencies, I figure that since yaml-cpp currently has no dependencies, 
there's a
good bit of inertia holding up that first one.

Original comment by jbe...@gmail.com on 11 Nov 2009 at 8:35

[GoogleCodeExporter]

Tim,

I don't know if you're still interested in following this up, but I was playing 
around with getting rid of some Visual Studio warnings, and I came to the line

    return stream >> output;

You commented earlier: "Don't use fail, it doesn't do what you think it does. 
You 
really only care about the result of _this_ extraction."

I believe (and correct me if I'm wrong) that:

1. `stream >> output` returns `stream`
2. In order to convert to `bool`, it first converts to `void *`.

Now, `operator void *` returns a null pointer if either one of the error flags 
(failbit or badbit) is set. But `fail()` *also* returns true if either the 
failbit or 
the badbit is set. (http://www.cplusplus.com/reference/iostream/istream/)

Am I missing something? Why can't I use `fail()` here?

Original comment by jbe...@gmail.com on 3 Mar 2010 at 5:19

[GoogleCodeExporter]

Hmm.  I thought there was a difference there, I probably confused checking 
good() (which also checks for 
eof).  I tried a small testbed on xcode here at home just now and fail appears 
to be equivalent.  I'll run it on 
VC Express and mingw later this week to see if the test works the same.

Original comment by tim.fi...@gmail.com on 3 Mar 2010 at 6:24