amorlzu / pugixml

Automatically exported from code.google.com/p/pugixml
0 stars 0 forks source link

Methods xml_attribute::as_* and xml_text::as_* should use default value when read value couldn't be parsed. #237

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
For example function get_value_int returns 0 when invalid integer string was 
provided. It is an error return value of function strtol.

In my opinion there should be another checking of results to allow user to 
handle or to determine which attributes was invalid. Example implementation:

PUGI__FN int get_value_int(const char_t* value, int def)
{
    if (!value) return def;

    int base = get_integer_base(value);

    char_t * pEnd = 0;
    errno = 0;

#ifdef PUGIXML_WCHAR_MODE
    int val = static_cast<int>(wcstol(value, &pEnd, base));
#else
    int val = static_cast<int>(strtol(value, &pEnd, base));

#endif

    if ((pEnd != 0 && *pEnd != 0) || errno != 0)
        return def;

    return val;
}

Original issue reported on code.google.com by tomasz.c...@gmail.com on 7 Aug 2014 at 8:18

GoogleCodeExporter commented 9 years ago
I'm hesitant to use errno since on some (admittedly broken) runtime 
implementations errno is not thread-local so pugixml accessors can spuriously 
return default values if there are failures in another thread.

Also resetting errno potentially leads to changing behavior of calling code if 
it checks errno across pugixml call boundary. Not resetting errno is impossible 
since then the function will spuriously fail...

Checking pEnd makes sense though.

Original comment by arseny.k...@gmail.com on 25 Aug 2014 at 5:35

GoogleCodeExporter commented 9 years ago

Original comment by arseny.k...@gmail.com on 25 Aug 2014 at 5:35

GoogleCodeExporter commented 9 years ago
Instead of resetting errno we can check if errno has changed to for example 
ERANGE but there is still problem with a multi-threading.

When parsing float value we should check if pEnd is 'f' to allow parsing reals 
in "1.2f" format.

Original comment by tomasz.c...@gmail.com on 26 Aug 2014 at 8:14

GoogleCodeExporter commented 9 years ago
Special treatment for 1.2f is worrisome. At this point I'm reluctant to 
implement this - this can be a breaking change if the application relied on 
being able to parse "1.2f".

I think there are two real options here - either keep the current behavior or 
implement an additional check that verifies the pEnd pointer but does not do 
any additional verification.

I'm moving the issue to GitHub: https://github.com/zeux/pugixml/issues/15

Original comment by arseny.k...@gmail.com on 26 Oct 2014 at 9:01