Re: [PATCH] xmllite: Avoid extra read from stream when identifying comment node.

[Nikolay Sivov]

On 11/20/19 6:23 PM, Jeff Smith wrote:
> On Wed, Nov 20, 2019 at 4:32 AM Nikolay Sivov  wrote:
> > On 11/19/19 4:40 AM, Jeff Smith wrote:
> > > Signed-off-by: Jeff Smith
> > >
> > > 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