Fix for issue 205: the string parser code overstepping to (or past) "end()" #206
Conversation
When parsing strings, the code sometimes oversteps the string iterator to end or past end and then dereferences without checking if it's valid to do so. This results in an exception being thrown in newer versions of Visual Studio. The change involves moving the code from passing a current position and end iterator to various functions. Instead, they're passed an object that tracks both of those and ensures that: 1. The iterator never steps past end 2. The iterator is only dereferenced when it is safe to do so This change also includes some cleanup of warnings on Windows
thinlang
changed the title
svgnative/CMakeLists.txt
Outdated
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4290 /wd4335 /wd4355 /wd4814 /wd4091 /TP") | ||
| if (NOT USE_SKIA) | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /std:c++11") | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /std:c++14") |
There was a problem hiding this comment.
The idea is to have an interoperable library that is able to go back to C++11. So a requirement for C++14 should be avoided.
We do have code that allows to switch to C++17 but keeping C++11 compatibility. See PR #194.
| } | ||
|
|
||
| SVG_ASSERT(radiusX == radiusY); // untested | ||
| //SVG_ASSERT(radiusX == radiusY); // untested |
There was a problem hiding this comment.
Why remove this assertion? Float comparison should include epsilon, but in general?
There was a problem hiding this comment.
This assert fired on my machine when running the tests on Windows locally
svgnative/src/SVGStringParser.cpp
Outdated
| template <typename T> | ||
| auto operator-(T) = delete; | ||
|
|
||
| [[nodiscard]] explicit operator bool() const |
There was a problem hiding this comment.
This is not compatible to C++11. If you think we should add it regardless, think about compatible ways like with such macro setup:
#if defined(__has_cpp_attribute)
# if __has_cpp_attribute(nodiscard) && __cplusplus >= 201703L
# define NODISCARD [[nodiscard]]
# elif defined(__GNUC__) || defined(__clang__)
# define NODISCARD __attribute__((warn_unused_result))
# elif defined(_MSC_VER)
# define NODISCARD _Check_return_
# else
# define NODISCARD
# endif
#else
# if defined(__GNUC__) || defined(__clang__)
# define NODISCARD __attribute__((warn_unused_result))
# elif defined(_MSC_VER)
# define NODISCARD _Check_return_
# else
# define NODISCARD
# endif
#endif
svgnative/src/SVGStringParser.cpp
Outdated
| return c >= '0' && c <= '9'; | ||
| } | ||
| template <typename T> | ||
| auto operator+(T) = delete; |
There was a problem hiding this comment.
This is C++20, no? So not valid in C++11 either.
| namespace SVGStringParser | ||
| { | ||
| using CharIt = std::string::const_iterator; | ||
| class CharPos final { |
There was a problem hiding this comment.
While I do like avoiding the endPos check, this can lead to a bigger performance degradation in a very performance critical path. We should run performance tests with huge path strings.
Description
When parsing strings, the code sometimes oversteps the string iterator to end or past end and then dereferences without checking if it's valid to do so. This results in an exception being thrown in newer versions of Visual Studio.
The change involves modifying functions that take a current position and end iterator. Instead, they're passed an object that tracks both of those and ensures that:
This change also includes some cleanup of warnings on Windows
Related Issue
#205
Motivation and Context
String parsing should not throw exceptions for reasonable or badly formed files.
How Has This Been Tested?
I ran the test suite locally on Windows per the readme instructions.
Screenshots (if appropriate):
Types of changes
Checklist: