[PATCH] xmllite: Avoid extra read from stream when identifying comment node.
Signed-off-by: Jeff Smith <whydoubt(a)gmail.com> --- dlls/xmllite/reader.c | 5 +++-- dlls/xmllite/tests/reader.c | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/xmllite/reader.c b/dlls/xmllite/reader.c index eddc4d8eec..5299871136 100644 --- a/dlls/xmllite/reader.c +++ b/dlls/xmllite/reader.c @@ -1459,7 +1459,7 @@ static HRESULT reader_parse_comment(xmlreader *reader) reader_init_strvalue(start, reader_get_cur(reader)-start, &value); TRACE("%s\n", debug_strval(reader, &value)); - /* skip rest of markup '->' */ + /* skip rest of markup '-->' */ reader_skipn(reader, 3); reader_set_strvalue(reader, StringValue_Value, &value); @@ -1472,8 +1472,9 @@ static HRESULT reader_parse_comment(xmlreader *reader) } } - reader_skipn(reader, 1); ptr++; + if (*ptr) + reader_skipn(reader, 1); } return S_OK; diff --git a/dlls/xmllite/tests/reader.c b/dlls/xmllite/tests/reader.c index 41adad1598..0bbdabbb96 100644 --- a/dlls/xmllite/tests/reader.c +++ b/dlls/xmllite/tests/reader.c @@ -1579,7 +1579,6 @@ static void test_read_pending(void) ok(hr == S_OK || broken(hr == E_PENDING), "got 0x%08x\n", hr); /* newer versions are happy when it's enough data to detect node type, older versions keep reading until it fails to read more */ -todo_wine ok(stream_readcall == 1 || broken(stream_readcall > 1), "got %d\n", stream_readcall); ok(type == XmlNodeType_Comment || broken(type == XmlNodeType_None), "got %d\n", type); -- 2.23.0
On 11/19/19 4:40 AM, Jeff Smith wrote:
Signed-off-by: Jeff Smith <whydoubt(a)gmail.com> --- dlls/xmllite/reader.c | 5 +++-- dlls/xmllite/tests/reader.c | 1 - 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/dlls/xmllite/reader.c b/dlls/xmllite/reader.c index eddc4d8eec..5299871136 100644 --- a/dlls/xmllite/reader.c +++ b/dlls/xmllite/reader.c @@ -1459,7 +1459,7 @@ static HRESULT reader_parse_comment(xmlreader *reader) reader_init_strvalue(start, reader_get_cur(reader)-start, &value); TRACE("%s\n", debug_strval(reader, &value));
- /* skip rest of markup '->' */ + /* skip rest of markup '-->' */ reader_skipn(reader, 3); That's obviously correct.
reader_set_strvalue(reader, StringValue_Value, &value); @@ -1472,8 +1472,9 @@ static HRESULT reader_parse_comment(xmlreader *reader) } }
- reader_skipn(reader, 1); ptr++; + if (*ptr) + reader_skipn(reader, 1); } I don't think it makes sense to change that just to get expected call sequence. Admittedly test itself is questionable.
On Wed, Nov 20, 2019 at 4:32 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 11/19/19 4:40 AM, Jeff Smith wrote:
Signed-off-by: Jeff Smith <whydoubt(a)gmail.com> --- dlls/xmllite/reader.c | 5 +++-- dlls/xmllite/tests/reader.c | 1 - 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/dlls/xmllite/reader.c b/dlls/xmllite/reader.c index eddc4d8eec..5299871136 100644 --- a/dlls/xmllite/reader.c +++ b/dlls/xmllite/reader.c @@ -1459,7 +1459,7 @@ static HRESULT reader_parse_comment(xmlreader *reader) reader_init_strvalue(start, reader_get_cur(reader)-start, &value); TRACE("%s\n", debug_strval(reader, &value));
- /* skip rest of markup '->' */ + /* skip rest of markup '-->' */ reader_skipn(reader, 3); That's obviously correct.
reader_set_strvalue(reader, StringValue_Value, &value); @@ -1472,8 +1472,9 @@ static HRESULT reader_parse_comment(xmlreader *reader) } }
- reader_skipn(reader, 1); ptr++; + if (*ptr) + reader_skipn(reader, 1); } I don't think it makes sense to change that just to get expected call sequence. Admittedly test itself is questionable.
Hi Nikolay, It appears that determining the value of the test is key here. It does seem to be bordering on 'implementation detail' territory. I think either the test should be removed, or the code should be fixed to pass the test. If the test is removed, there is no need for my patch. Otherwise, the test should pass, and I will defend my fix if need be. Thanks, Jeff
On 11/20/19 6:23 PM, Jeff Smith wrote:
On Wed, Nov 20, 2019 at 4:32 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 11/19/19 4:40 AM, Jeff Smith wrote:
Signed-off-by: Jeff Smith <whydoubt(a)gmail.com> --- dlls/xmllite/reader.c | 5 +++-- dlls/xmllite/tests/reader.c | 1 - 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/dlls/xmllite/reader.c b/dlls/xmllite/reader.c index eddc4d8eec..5299871136 100644 --- a/dlls/xmllite/reader.c +++ b/dlls/xmllite/reader.c @@ -1459,7 +1459,7 @@ static HRESULT reader_parse_comment(xmlreader *reader) reader_init_strvalue(start, reader_get_cur(reader)-start, &value); TRACE("%s\n", debug_strval(reader, &value));
- /* skip rest of markup '->' */ + /* skip rest of markup '-->' */ reader_skipn(reader, 3); That's obviously correct. reader_set_strvalue(reader, StringValue_Value, &value); @@ -1472,8 +1472,9 @@ static HRESULT reader_parse_comment(xmlreader *reader) } }
- reader_skipn(reader, 1); ptr++; + if (*ptr) + reader_skipn(reader, 1); } I don't think it makes sense to change that just to get expected call sequence. Admittedly test itself is questionable.
Hi Nikolay,
It appears that determining the value of the test is key here. It does seem to be bordering on 'implementation detail' territory.
I think either the test should be removed, or the code should be fixed to pass the test. If the test is removed, there is no need for my patch. Otherwise, the test should pass, and I will defend my fix if need be. I wouldn't mind if this test function was removed.
Thanks, Jeff
participants (2)
-
Jeff Smith -
Nikolay Sivov