Re: [PATCH] riched20: Remove duplicated condition and duplicated test

[Huw Davies]

On Tue, Jun 14, 2016 at 08:56:41PM +0300, Nikolay Sivov wrote:
> diff --git a/dlls/riched20/richole.c b/dlls/riched20/richole.c
> index 1013e67..a2f23c9 100644
> --- a/dlls/riched20/richole.c
> +++ b/dlls/riched20/richole.c
> @@ -1976,7 +1976,7 @@ static HRESULT range_Collapse(LONG bStart, LONG *start, LONG *end)
>    if (*end == *start)
>        return S_FALSE;
>
> -  if (bStart == tomEnd || bStart == tomFalse)
> +  if (bStart == tomEnd)
>        *start = *end;
>    else
>        *end = *start;
>
> diff --git a/dlls/riched20/tests/richole.c b/dlls/riched20/tests/richole.c
> index 6da3ce3..33043fd 100644
> --- a/dlls/riched20/tests/richole.c
> +++ b/dlls/riched20/tests/richole.c
> @@ -1361,18 +1361,6 @@ static void test_ITextRange_Collapse(void)
>    first = 4, lim = 8;
>    hres = ITextDocument_Range(txtDoc, first, lim, &txtRge);
>    ok(hres == S_OK, "got 0x%08x\n", hres);
> -  hres = ITextRange_Collapse(txtRge, tomTrue);
> -  ok(hres == S_OK, "ITextRange_Collapse\n");
> -  hres = ITextRange_GetStart(txtRge, &start);
> -  ok(hres == S_OK, "got 0x%08x\n", hres);
> -  ok(start == 4, "got wrong start value: %d\n", start);
> -  hres = ITextRange_GetEnd(txtRge, &end);
> -  ok(hres == S_OK, "got 0x%08x\n", hres);
> -  ok(end == 4, "got wrong end value: %d\n", end);
> -  ITextRange_Release(txtRge);
> -
> -  hres = ITextDocument_Range(txtDoc, first, lim, &txtRge);
> -  ok(hres == S_OK, "got 0x%08x\n", hres);
>    hres = ITextRange_Collapse(txtRge, tomStart);
>    ok(hres == S_OK, "ITextRange_Collapse\n");
>    hres = ITextRange_GetStart(txtRge, &start);
> @@ -1385,18 +1373,6 @@ static void test_ITextRange_Collapse(void)
>
> hres = ITextDocument_Range(txtDoc, first, lim, &txtRge);
>    ok(hres == S_OK, "got 0x%08x\n", hres);
> -  hres = ITextRange_Collapse(txtRge, tomFalse);
> -  ok(hres == S_OK, "ITextRange_Collapse\n");
> -  hres = ITextRange_GetStart(txtRge, &start);
> -  ok(hres == S_OK, "got 0x%08x\n", hres);
> -  ok(start == 8, "got wrong start value: %d\n", start);
> -  hres = ITextRange_GetEnd(txtRge, &end);
> -  ok(hres == S_OK, "got 0x%08x\n", hres);
> -  ok(end == 8, "got wrong end value: %d\n", end);
> -  ITextRange_Release(txtRge);
> -
> -  hres = ITextDocument_Range(txtDoc, first, lim, &txtRge);
> -  ok(hres == S_OK, "got 0x%08x\n", hres);
>    hres = ITextRange_Collapse(txtRge, tomEnd);
>    ok(hres == S_OK, "ITextRange_Collapse\n");
>    hres = ITextRange_GetStart(txtRge, &start);

I don't think we should remove the extra tests, especially as
tomTrue != tomStart and it's documentented to work with either.
Leaving the tomFalse tests in is fine too.

Huw.

[PATCH] riched20: Remove duplicated condition and duplicated test

[Nikolay Sivov]

On 15.06.2016 10:43, Huw Davies wrote:
> On Tue, Jun 14, 2016 at 08:56:41PM +0300, Nikolay Sivov wrote:
> > diff --git a/dlls/riched20/richole.c b/dlls/riched20/richole.c
> > index 1013e67..a2f23c9 100644
> > --- a/dlls/riched20/richole.c
> > +++ b/dlls/riched20/richole.c
> > @@ -1976,7 +1976,7 @@ static HRESULT range_Collapse(LONG bStart, LONG *start, LONG *end)
> >    if (*end == *start)
> >        return S_FALSE;
> >
> > -  if (bStart == tomEnd || bStart == tomFalse)
> > +  if (bStart == tomEnd)
> >        *start = *end;
> >    else
> >        *end = *start;
> >
> > diff --git a/dlls/riched20/tests/richole.c b/dlls/riched20/tests/richole.c
> > index 6da3ce3..33043fd 100644
> > --- a/dlls/riched20/tests/richole.c
> > +++ b/dlls/riched20/tests/richole.c
> > @@ -1361,18 +1361,6 @@ static void test_ITextRange_Collapse(void)
> >    first = 4, lim = 8;
> >    hres = ITextDocument_Range(txtDoc, first, lim, &txtRge);
> >    ok(hres == S_OK, "got 0x%08x\n", hres);
> > -  hres = ITextRange_Collapse(txtRge, tomTrue);
> > -  ok(hres == S_OK, "ITextRange_Collapse\n");
> > -  hres = ITextRange_GetStart(txtRge, &start);
> > -  ok(hres == S_OK, "got 0x%08x\n", hres);
> > -  ok(start == 4, "got wrong start value: %d\n", start);
> > -  hres = ITextRange_GetEnd(txtRge, &end);
> > -  ok(hres == S_OK, "got 0x%08x\n", hres);
> > -  ok(end == 4, "got wrong end value: %d\n", end);
> > -  ITextRange_Release(txtRge);
> > -
> > -  hres = ITextDocument_Range(txtDoc, first, lim, &txtRge);
> > -  ok(hres == S_OK, "got 0x%08x\n", hres);
> >    hres = ITextRange_Collapse(txtRge, tomStart);
> >    ok(hres == S_OK, "ITextRange_Collapse\n");
> >    hres = ITextRange_GetStart(txtRge, &start);
> > @@ -1385,18 +1373,6 @@ static void test_ITextRange_Collapse(void)
> >
> > hres = ITextDocument_Range(txtDoc, first, lim, &txtRge);
> >    ok(hres == S_OK, "got 0x%08x\n", hres);
> > -  hres = ITextRange_Collapse(txtRge, tomFalse);
> > -  ok(hres == S_OK, "ITextRange_Collapse\n");
> > -  hres = ITextRange_GetStart(txtRge, &start);
> > -  ok(hres == S_OK, "got 0x%08x\n", hres);
> > -  ok(start == 8, "got wrong start value: %d\n", start);
> > -  hres = ITextRange_GetEnd(txtRge, &end);
> > -  ok(hres == S_OK, "got 0x%08x\n", hres);
> > -  ok(end == 8, "got wrong end value: %d\n", end);
> > -  ITextRange_Release(txtRge);
> > -
> > -  hres = ITextDocument_Range(txtDoc, first, lim, &txtRge);
> > -  ok(hres == S_OK, "got 0x%08x\n", hres);
> >    hres = ITextRange_Collapse(txtRge, tomEnd);
> >    ok(hres == S_OK, "ITextRange_Collapse\n");
> >    hres = ITextRange_GetStart(txtRge, &start);
>
> I don't think we should remove the extra tests, especially as
> tomTrue != tomStart and it's documentented to work with either.
> Leaving the tomFalse tests in is fine too.

Okay, sent version 2 without test changes.
> Huw.