Qpid
  1. Qpid
  2. QPID-3206

Variant converts from negative number in string format to unsigned integer without error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.8, 0.10
    • Fix Version/s: 0.11
    • Component/s: C++ Broker, C++ Client
    • Labels:
      None

      Description

      E.g.
      uint16_t i = 0;
      qpid::types::Variant v = "-5";
      i = v;

      The above results in an exception in older versions of gcc (e.g. 4.1.2) but sets i to 65531 on later versions (e.g. 4.4.4). This is a result of a fix to the gcc std library to be compliant with specification which requires stringstream to accept negative values even for unsigned ints (which is the behaviour of scanf).

      See e.g:
      http://boost.2283326.n4.nabble.com/conversion-lexical-cast-doesn-t-throw-td2593967.html
      http://groups.google.com/group/comp.lang.c++.moderated/browse_thread/thread/97475b21515462c9/ce369a327fa39243#ce369a327fa39243

        Activity

        Gordon Sim created issue -
        Hide
        Gordon Sim added a comment -

        Suggested change to retain behaviour of older gcc: https://reviews.apache.org/r/596/

        Show
        Gordon Sim added a comment - Suggested change to retain behaviour of older gcc: https://reviews.apache.org/r/596/
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/
        -----------------------------------------------------------

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary
        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.
        https://issues.apache.org/jira/browse/QPID-3206

        Diffs


        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157
        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing
        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review441
        -----------------------------------------------------------

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp
        <https://reviews.apache.org/r/596/#comment864>

        This change is based on the assumption that convertFromString is only called for numeric types of T. Does this do the right thing for floating point numbers? Does is_signed always return true for floats and doubles?

        • Ted

        On 2011-04-13 10:49:00, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 10:49:00)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review441 ----------------------------------------------------------- /trunk/qpid/cpp/src/qpid/types/Variant.cpp < https://reviews.apache.org/r/596/#comment864 > This change is based on the assumption that convertFromString is only called for numeric types of T. Does this do the right thing for floating point numbers? Does is_signed always return true for floats and doubles? Ted On 2011-04-13 10:49:00, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 10:49:00) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-04-13 12:43:57, Ted Ross wrote:

        > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 114

        > <https://reviews.apache.org/r/596/diff/1/?file=15701#file15701line114>

        >

        > This change is based on the assumption that convertFromString is only called for numeric types of T. Does this do the right thing for floating point numbers? Does is_signed always return true for floats and doubles?

        >

        Yes. it appears to do the right thing and I would certainly expect is_signed to always return true for floats/doubles. I've added another couple of test cases.

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review441
        -----------------------------------------------------------

        On 2011-04-13 13:00:09, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 13:00:09)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-04-13 12:43:57, Ted Ross wrote: > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 114 > < https://reviews.apache.org/r/596/diff/1/?file=15701#file15701line114 > > > This change is based on the assumption that convertFromString is only called for numeric types of T. Does this do the right thing for floating point numbers? Does is_signed always return true for floats and doubles? > Yes. it appears to do the right thing and I would certainly expect is_signed to always return true for floats/doubles. I've added another couple of test cases. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review441 ----------------------------------------------------------- On 2011-04-13 13:00:09, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 13:00:09) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/
        -----------------------------------------------------------

        (Updated 2011-04-13 13:00:09.156587)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Changes
        -------

        Added test cases from string to float/double conversion when the value is negative.

        Summary
        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.
        https://issues.apache.org/jira/browse/QPID-3206

        Diffs (updated)


        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157
        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing
        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 13:00:09.156587) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Changes ------- Added test cases from string to float/double conversion when the value is negative. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs (updated) /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review443
        -----------------------------------------------------------

        Ship it!

        • Kenneth

        On 2011-04-13 13:00:09, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 13:00:09)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review443 ----------------------------------------------------------- Ship it! Kenneth On 2011-04-13 13:00:09, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 13:00:09) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review444
        -----------------------------------------------------------

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp
        <https://reviews.apache.org/r/596/#comment866>

        I think this test will do the wrong thing for -0.

        It's an annoying corner case but it does have a correct unsigned representation!

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp
        <https://reviews.apache.org/r/596/#comment867>

        I think you need a comment to explain that an exception will be thrown below, so that someone modifying this code won't lose the thrown exception.

        • Andrew

        On 2011-04-13 13:00:09, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 13:00:09)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review444 ----------------------------------------------------------- /trunk/qpid/cpp/src/qpid/types/Variant.cpp < https://reviews.apache.org/r/596/#comment866 > I think this test will do the wrong thing for -0. It's an annoying corner case but it does have a correct unsigned representation! /trunk/qpid/cpp/src/qpid/types/Variant.cpp < https://reviews.apache.org/r/596/#comment867 > I think you need a comment to explain that an exception will be thrown below, so that someone modifying this code won't lose the thrown exception. Andrew On 2011-04-13 13:00:09, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 13:00:09) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/
        -----------------------------------------------------------

        (Updated 2011-04-13 15:11:45.041621)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Changes
        -------

        Added comment to clarify that flow of control leads to exception, as requested by Andrew.

        Summary
        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.
        https://issues.apache.org/jira/browse/QPID-3206

        Diffs (updated)


        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157
        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing
        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 15:11:45.041621) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Changes ------- Added comment to clarify that flow of control leads to exception, as requested by Andrew. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs (updated) /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-04-13 14:44:16, Andrew Stitcher wrote:

        > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 118

        > <https://reviews.apache.org/r/596/diff/2/?file=15703#file15703line118>

        >

        > I think you need a comment to explain that an exception will be thrown below, so that someone modifying this code won't lose the thrown exception.

        Agreed. New patch uploaded that does so.

        On 2011-04-13 14:44:16, Andrew Stitcher wrote:

        > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 114

        > <https://reviews.apache.org/r/596/diff/2/?file=15703#file15703line114>

        >

        > I think this test will do the wrong thing for -0.

        >

        > It's an annoying corner case but it does have a correct unsigned representation!

        Interestingly -0 fails to convert to an unsigned int in the existing code on the older compilers.

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review444
        -----------------------------------------------------------

        On 2011-04-13 15:11:45, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 15:11:45)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-04-13 14:44:16, Andrew Stitcher wrote: > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 118 > < https://reviews.apache.org/r/596/diff/2/?file=15703#file15703line118 > > > I think you need a comment to explain that an exception will be thrown below, so that someone modifying this code won't lose the thrown exception. Agreed. New patch uploaded that does so. On 2011-04-13 14:44:16, Andrew Stitcher wrote: > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 114 > < https://reviews.apache.org/r/596/diff/2/?file=15703#file15703line114 > > > I think this test will do the wrong thing for -0. > > It's an annoying corner case but it does have a correct unsigned representation! Interestingly -0 fails to convert to an unsigned int in the existing code on the older compilers. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review444 ----------------------------------------------------------- On 2011-04-13 15:11:45, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 15:11:45) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review447
        -----------------------------------------------------------

        Ship it!

        I think the extra tests below would improve the test coverage a bit, but not essential.

        /trunk/qpid/cpp/src/tests/Variant.cpp
        <https://reviews.apache.org/r/596/#comment871>

        Some things we don't check here that should also fail (essentially values that are valid as uint but not int or negative numbers too large.

        maxint<int64_t>+1 in a string -> int64_t
        [should fail]
        minint<int64_t>-1 in a string -> int64_t
        [should fail]

        and for int32_t too

        • Andrew

        On 2011-04-13 15:11:45, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 15:11:45)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review447 ----------------------------------------------------------- Ship it! I think the extra tests below would improve the test coverage a bit, but not essential. /trunk/qpid/cpp/src/tests/Variant.cpp < https://reviews.apache.org/r/596/#comment871 > Some things we don't check here that should also fail (essentially values that are valid as uint but not int or negative numbers too large. maxint<int64_t>+1 in a string -> int64_t [should fail] minint<int64_t>-1 in a string -> int64_t [should fail] and for int32_t too Andrew On 2011-04-13 15:11:45, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 15:11:45) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-04-13 14:44:16, Andrew Stitcher wrote:

        > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 114

        > <https://reviews.apache.org/r/596/diff/2/?file=15703#file15703line114>

        >

        > I think this test will do the wrong thing for -0.

        >

        > It's an annoying corner case but it does have a correct unsigned representation!

        Gordon Sim wrote:

        Interestingly -0 fails to convert to an unsigned int in the existing code on the older compilers.

        Fixed now.

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review444
        -----------------------------------------------------------

        On 2011-04-13 16:13:56, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 16:13:56)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-04-13 14:44:16, Andrew Stitcher wrote: > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 114 > < https://reviews.apache.org/r/596/diff/2/?file=15703#file15703line114 > > > I think this test will do the wrong thing for -0. > > It's an annoying corner case but it does have a correct unsigned representation! Gordon Sim wrote: Interestingly -0 fails to convert to an unsigned int in the existing code on the older compilers. Fixed now. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review444 ----------------------------------------------------------- On 2011-04-13 16:13:56, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 16:13:56) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/
        -----------------------------------------------------------

        (Updated 2011-04-13 16:13:56.238125)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Changes
        -------

        Added new tests. Fixed special case of -0.

        Summary
        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.
        https://issues.apache.org/jira/browse/QPID-3206

        Diffs (updated)


        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157
        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing
        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 16:13:56.238125) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Changes ------- Added new tests. Fixed special case of -0. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs (updated) /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-04-13 15:22:31, Andrew Stitcher wrote:

        > /trunk/qpid/cpp/src/tests/Variant.cpp, line 109

        > <https://reviews.apache.org/r/596/diff/3/?file=15706#file15706line109>

        >

        > Some things we don't check here that should also fail (essentially values that are valid as uint but not int or negative numbers too large.

        >

        > maxint<int64_t>+1 in a string -> int64_t

        > [should fail]

        > minint<int64_t>-1 in a string -> int64_t

        > [should fail]

        >

        > and for int32_t too

        >

        >

        Added in latest patch.

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review447
        -----------------------------------------------------------

        On 2011-04-13 16:13:56, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 16:13:56)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-04-13 15:22:31, Andrew Stitcher wrote: > /trunk/qpid/cpp/src/tests/Variant.cpp, line 109 > < https://reviews.apache.org/r/596/diff/3/?file=15706#file15706line109 > > > Some things we don't check here that should also fail (essentially values that are valid as uint but not int or negative numbers too large. > > maxint<int64_t>+1 in a string -> int64_t > [should fail] > minint<int64_t>-1 in a string -> int64_t > [should fail] > > and for int32_t too > > Added in latest patch. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review447 ----------------------------------------------------------- On 2011-04-13 16:13:56, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 16:13:56) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review450
        -----------------------------------------------------------

        Ship it!

        • Steve

        On 2011-04-13 16:13:56, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 16:13:56)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review450 ----------------------------------------------------------- Ship it! Steve On 2011-04-13 16:13:56, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 16:13:56) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review451
        -----------------------------------------------------------

        Another useful test would be to convert a float with a "-" not at the first position something like "12e-3" -> double or float.
        [Just to be sure the find("-") doesn't kick in unexpectedly]

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp
        <https://reviews.apache.org/r/596/#comment874>

        This won't allow the (stupid but) legal -00 or -000 etc!

        Perhaps a better test would be recording you saw a "-" and doing the conversion anyway, then only throwing if the result was non zero.

        something like:

        bool n = s->find('-') != 0;
        T r = boost::lexical_cast<T>(*s);
        if (n && r != 0)

        { throw InvalidConversion(...); }
        • Andrew

        On 2011-04-13 16:13:56, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 16:13:56)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review451 ----------------------------------------------------------- Another useful test would be to convert a float with a "-" not at the first position something like "12e-3" -> double or float. [Just to be sure the find("-") doesn't kick in unexpectedly] /trunk/qpid/cpp/src/qpid/types/Variant.cpp < https://reviews.apache.org/r/596/#comment874 > This won't allow the (stupid but) legal -00 or -000 etc! Perhaps a better test would be recording you saw a "-" and doing the conversion anyway, then only throwing if the result was non zero. something like: bool n = s->find('-') != 0; T r = boost::lexical_cast<T>(*s); if (n && r != 0) { throw InvalidConversion(...); } Andrew On 2011-04-13 16:13:56, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 16:13:56) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-04-13 14:44:16, Andrew Stitcher wrote:

        > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 114

        > <https://reviews.apache.org/r/596/diff/2/?file=15703#file15703line114>

        >

        > I think this test will do the wrong thing for -0.

        >

        > It's an annoying corner case but it does have a correct unsigned representation!

        Gordon Sim wrote:

        Interestingly -0 fails to convert to an unsigned int in the existing code on the older compilers.

        Gordon Sim wrote:

        Fixed now.

        Ok then, I suppose it depends how finicky we want to be!

        • Andrew

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review444
        -----------------------------------------------------------

        On 2011-04-13 16:13:56, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 16:13:56)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-04-13 14:44:16, Andrew Stitcher wrote: > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 114 > < https://reviews.apache.org/r/596/diff/2/?file=15703#file15703line114 > > > I think this test will do the wrong thing for -0. > > It's an annoying corner case but it does have a correct unsigned representation! Gordon Sim wrote: Interestingly -0 fails to convert to an unsigned int in the existing code on the older compilers. Gordon Sim wrote: Fixed now. Ok then, I suppose it depends how finicky we want to be! Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review444 ----------------------------------------------------------- On 2011-04-13 16:13:56, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 16:13:56) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/
        -----------------------------------------------------------

        (Updated 2011-04-13 17:53:50.358454)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Changes
        -------

        Revise handling of negative zero to also catch -000 etc

        Summary
        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.
        https://issues.apache.org/jira/browse/QPID-3206

        Diffs (updated)


        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157
        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing
        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 17:53:50.358454) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Changes ------- Revise handling of negative zero to also catch -000 etc Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs (updated) /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review454
        -----------------------------------------------------------

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp
        <https://reviews.apache.org/r/596/#comment879>

        Don't need this constant now

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp
        <https://reviews.apache.org/r/596/#comment880>

        Could still cast to T as it will be 0 if it is correct and any non zero value is an error in this branch.

        • Andrew

        On 2011-04-13 17:53:50, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 17:53:50)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review454 ----------------------------------------------------------- /trunk/qpid/cpp/src/qpid/types/Variant.cpp < https://reviews.apache.org/r/596/#comment879 > Don't need this constant now /trunk/qpid/cpp/src/qpid/types/Variant.cpp < https://reviews.apache.org/r/596/#comment880 > Could still cast to T as it will be 0 if it is correct and any non zero value is an error in this branch. Andrew On 2011-04-13 17:53:50, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 17:53:50) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-04-13 18:17:31, Andrew Stitcher wrote:

        > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 37

        > <https://reviews.apache.org/r/596/diff/5/?file=15709#file15709line37>

        >

        > Don't need this constant now

        Good catch, will remove.

        On 2011-04-13 18:17:31, Andrew Stitcher wrote:

        > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 126

        > <https://reviews.apache.org/r/596/diff/5/?file=15709#file15709line126>

        >

        > Could still cast to T as it will be 0 if it is correct and any non zero value is an error in this branch.

        You mean 'boost::lexical_cast<T>(*s)'? That will fail for -0 on older gcc compilers.

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review454
        -----------------------------------------------------------

        On 2011-04-13 17:53:50, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 17:53:50)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-04-13 18:17:31, Andrew Stitcher wrote: > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 37 > < https://reviews.apache.org/r/596/diff/5/?file=15709#file15709line37 > > > Don't need this constant now Good catch, will remove. On 2011-04-13 18:17:31, Andrew Stitcher wrote: > /trunk/qpid/cpp/src/qpid/types/Variant.cpp, line 126 > < https://reviews.apache.org/r/596/diff/5/?file=15709#file15709line126 > > > Could still cast to T as it will be 0 if it is correct and any non zero value is an error in this branch. You mean 'boost::lexical_cast<T>(*s)'? That will fail for -0 on older gcc compilers. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review454 ----------------------------------------------------------- On 2011-04-13 17:53:50, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 17:53:50) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review456
        -----------------------------------------------------------

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp
        <https://reviews.apache.org/r/596/#comment883>

        good point.

        • Andrew

        On 2011-04-13 17:53:50, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 17:53:50)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review456 ----------------------------------------------------------- /trunk/qpid/cpp/src/qpid/types/Variant.cpp < https://reviews.apache.org/r/596/#comment883 > good point. Andrew On 2011-04-13 17:53:50, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 17:53:50) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/596/#review460
        -----------------------------------------------------------

        Ship it!

        • Alan

        On 2011-04-13 17:53:50, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/596/

        -----------------------------------------------------------

        (Updated 2011-04-13 17:53:50)

        Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston.

        Summary

        -------

        Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.)

        This addresses bug QPID-3206.

        https://issues.apache.org/jira/browse/QPID-3206

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157

        /trunk/qpid/cpp/src/tests/Variant.cpp 1090157

        Diff: https://reviews.apache.org/r/596/diff

        Testing

        -------

        New tests included in patch

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/#review460 ----------------------------------------------------------- Ship it! Alan On 2011-04-13 17:53:50, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/596/ ----------------------------------------------------------- (Updated 2011-04-13 17:53:50) Review request for Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted Ross, and Steve Huston. Summary ------- Add special test for negative numeric strings when converting to unsigned values. (It appears - though I could be wrong - that in basic form stringstream does not do any special handling of negatives for different locales. That appears only to be done for money values.) This addresses bug QPID-3206 . https://issues.apache.org/jira/browse/QPID-3206 Diffs ----- /trunk/qpid/cpp/src/qpid/types/Variant.cpp 1090157 /trunk/qpid/cpp/src/tests/Variant.cpp 1090157 Diff: https://reviews.apache.org/r/596/diff Testing ------- New tests included in patch Thanks, Gordon
        Gordon Sim made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Justin Ross made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Gordon Sim
            Reporter:
            Gordon Sim
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development