re: ntdll: rtlstr.c: Implement checking for control characters in RtlIsTextUnicode [try 2]

[Dan Kegel]

The array doesn't make much sense if you're not going to iterate over it.
It might be more readable if you replaced the array and long if
with a switch statement.
It would be nice to break out of the loop early.
The last two suggestions together kind of mean a goto, which is ugly,
but not too uncommon in wine.
(You might want to verify that the generated code isn't too slow,
but since len is limited to 256, that probably doesn't matter.)

e.g.

if (flags & IS_TEXT_UNICODE_CONTROLS)
    for (i = 0; i < len; i++)
           switch (s[i]) {
           case '\t':
           case '\n':
           case 'r':
           case 0x20:
                out_flags |= IS_TEXT_UNICODE_CONTROLS;
                goto done;
           default:
           }
    }
    done:

ntdll: rtlstr.c: Implement checking for control characters in RtlIsTextUnicode [try 2]

[Zac Brown]

Dan Kegel wrote:
> The array doesn't make much sense if you're not going to iterate over it.
> It might be more readable if you replaced the array and long if
> with a switch statement.
> It would be nice to break out of the loop early.
> The last two suggestions together kind of mean a goto, which is ugly,
> but not too uncommon in wine.
> (You might want to verify that the generated code isn't too slow,
> but since len is limited to 256, that probably doesn't matter.)
>
> e.g.
> if (flags & IS_TEXT_UNICODE_CONTROLS)
>
> for (i = 0; i < len; i++)
>            switch (s[i]) {
>            case '\t':
>            case '\n':
>            case 'r':
>            case 0x20:
>                 out_flags |= IS_TEXT_UNICODE_CONTROLS;
>                 goto done;
>            default:
>            }
>     }
>     done:

I had thought about that before and initially wasn't sure of AJ's feelings on 
not declaring something that (I thought) might be considered magic values.

Now that I realized it needs to be optimized, I'll definitely look into it.

-Zac

ntdll: rtlstr.c: Implement checking for control characters inRtlIsTextUnicode [try 2]

[Rolf Kalbermatter]

Dan Kegel [mailto:[e-mail]]
> e.g.
> if (flags & IS_TEXT_UNICODE_CONTROLS)
>
> for (i = 0; i < len; i++)
>            switch (s[i]) {
>            case '\t':
>            case '\n':
>            case 'r':
>            case 0x20:
>                 out_flags |= IS_TEXT_UNICODE_CONTROLS;
>                 goto done;
>            default:
>            }
>     }
>     done:

Shouldn't a break instead of the goto work too?

Rolf Kalbermatter

ntdll: rtlstr.c: Implement checking for control characters inRtlIsTextUnicode [try 2]

[Zac Brown]

Rolf Kalbermatter wrote:
> Dan Kegel [mailto:[e-mail]]
> > e.g.
> > if (flags & IS_TEXT_UNICODE_CONTROLS)
> >
> > for (i = 0; i < len; i++)
> >            switch (s[i]) {
> >            case '\t':
> >            case '\n':
> >            case 'r':
> >            case 0x20:
> >                 out_flags |= IS_TEXT_UNICODE_CONTROLS;
> >                 goto done;
> >            default:
> >            }
> >     }
> >     done:
>
> Shouldn't a break instead of the goto work too?
>
> Rolf Kalbermatter

A break will only exit the switch statement which is pointless since we don't 
care about the rest of the string if we find a control character.

I made that mistake in my third try of the patch, didn't even think about it 
until Dan mentioned somethign to me.

Hopefully the 4th try will actually be decent.

-Zac

ntdll: rtlstr.c: Implement checking for control characters inRtlIsTextUnicode [try 2]

[Rolf Kalbermatter]

Zac Brown [mailto:[e-mail]]
> Rolf Kalbermatter wrote:
> > Dan Kegel [mailto:[e-mail]]
> > > e.g.
> > >
> > > if (flags & IS_TEXT_UNICODE_CONTROLS)
> > >        for (i = 0; i < len; i++)
> > >          switch (s[i]) {
> > >            case '\t':
> > >            case '\n':
> > >            case 'r':
> > >            case 0x20:
> > >                 out_flags |= IS_TEXT_UNICODE_CONTROLS;
> > >                 goto done;
> > >            default:
> > >          }
> > >     }
> > >     done:
> >
> > Shouldn't a break instead of the goto work too?
>
> A break will only exit the switch statement which is 
> pointless since we don't care about the rest of the string if 
> we find a control character.

Ah well, you are right of course! But personally I feel about the use
of a switch statement in such a case a bit "overdone". It's not so much
a selection of operations based on a variable as much more a clear
conditional expression such as

if (expression1 OR
        expresssion2 OR
        ...)
    {
       out_flags |= IS_TEXT_UNICODE_CONTROLS;
       break;
    }

which I think Dan was trying to avoid since he might not like "complex"
conditional expressions. But then I would normally go myself to great
lengths to avoid a goto as I feel bad about the use of them :-)

The fact that s[i] would seem to be evaluated 4 times might look bad
but should be no problem for nowadays optimizing compilers to create
similar code as what a switch statement would produce and could be
also avoided by an incremented character pointer instead of the loop
counter i, which might be even more optimal.

Rolf Kalbermatter

ntdll: rtlstr.c: Implement checking for control characters inRtlIsTextUnicode [try 2]

[Dan Kegel]

On Fri, Jun 20, 2008 at 12:23 AM, Rolf Kalbermatter
 wrote:
> But personally I feel about the use
> of a switch statement in such a case a bit "overdone". It's not so much
> a selection of operations based on a variable as much more a clear
> conditional expression

I was going for maximally concise and beautiful.  A long conditional
is fine, too.
> I would normally go myself to great
> lengths to avoid a goto as I feel bad about the use of them :-)

Me, too, except in wine and the kernel, where the language
is missing a construct (e.g. C needs a more powerful break),
and sometimes in error handling.
> The fact that s[i] would seem to be evaluated 4 times might look bad
> but should be no problem for nowadays optimizing compilers to create
> similar code as what a switch statement would produce and could be

I looked -- oddly, the switch seems to produce smaller code.
Performance probably doesn't matter in this function, though.
> also avoided by an incremented character pointer instead of the loop
> counter i, which might be even more optimal.

Modern compilers like array indices more than they like pointers,
believe it or not.

And premature optimization is the root of all evil; we should
probably go for the most concise and readable code first,
and avoid inefficiency second.  (So it's ok to hoist the endian
check out of the loop, even though that's an optimization,
because to me it makes the code more natural...)

This is way more discussion of coding than we normally should
ever have about a patch.  I'll shut up now :-)
- Dan