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

[Michael Karcher]

Am Donnerstag, den 19.06.2008, 12:52 -0700 schrieb Zac Brown:
> - Because these tests are mutually exclusive, a single goto label suffices for 
> breaking the loop.

As written, your tests are not mutually exclusive. It could happen that
flags has IS_TEXT_UNICODE_CONTROLS and IS_TEXT_UNICODE_REVERSE_CONTROLS,
set and out_flags contains IS_TEXT_UNICODE_REVERSE_SIGNATURE. (If the
caller doesn't provide flags, both flags are set, in fact!) In that
case, both conditions of your if statements are true.

I don't know whether that means that you need two goto targets or that
your logic should be changed, but I suspect the latter. If you test for
reversed control characters only I a reversed BOM was found, you should
not test for non-reversed control characters if a reversed BOM was
found, according to my intuition.

Regards,
  Michael Karcher

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

[Zac Brown]

Michael Karcher wrote:
> Am Donnerstag, den 19.06.2008, 12:52 -0700 schrieb Zac Brown:
> > - Because these tests are mutually exclusive, a single goto label suffices for 
> > breaking the loop.
>
> As written, your tests are not mutually exclusive. It could happen that
> flags has IS_TEXT_UNICODE_CONTROLS and IS_TEXT_UNICODE_REVERSE_CONTROLS,
> set and out_flags contains IS_TEXT_UNICODE_REVERSE_SIGNATURE. (If the
> caller doesn't provide flags, both flags are set, in fact!) In that
> case, both conditions of your if statements are true.
>
> I don't know whether that means that you need two goto targets or that
> your logic should be changed, but I suspect the latter. If you test for
> reversed control characters only I a reversed BOM was found, you should
> not test for non-reversed control characters if a reversed BOM was
> found, according to my intuition.
>
> Regards,
>   Michael Karcher

Hmmm this is true. Added a test in the first if statement to do something like:

if((flags & IS_TEXT_UNICODE_CONTROL) && (out_flags & IS_TEXT_UNICODE_SIGNATRUE)) 
causes a wine test to fail.

So either I need a goto at the end of the first if statement before the closing 
brace, or I need to adjust some other things in the code.

Any thoughts from anyone on the proper way to approach this ;). I'd like to get 
this one done before try 10 (haha).

-Zac

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

[Zac Brown]

Zac Brown wrote:
> Michael Karcher wrote:
> > Am Donnerstag, den 19.06.2008, 12:52 -0700 schrieb Zac Brown:
> > > - Because these tests are mutually exclusive, a single goto label suffices for 
> > > breaking the loop.
> > > As written, your tests are not mutually exclusive. It could happen that
> > > flags has IS_TEXT_UNICODE_CONTROLS and IS_TEXT_UNICODE_REVERSE_CONTROLS,
> > > set and out_flags contains IS_TEXT_UNICODE_REVERSE_SIGNATURE. (If the
> > > caller doesn't provide flags, both flags are set, in fact!) In that
> > > case, both conditions of your if statements are true.
> >
> > I don't know whether that means that you need two goto targets or that
> > your logic should be changed, but I suspect the latter. If you test for
> > reversed control characters only I a reversed BOM was found, you should
> > not test for non-reversed control characters if a reversed BOM was
> > found, according to my intuition.
> >
> > Regards,
> >   Michael Karcher
>
> Hmmm this is true. Added a test in the first if statement to do something like:
>
> if((flags & IS_TEXT_UNICODE_CONTROL) && (out_flags & IS_TEXT_UNICODE_SIGNATRUE)) 
> causes a wine test to fail.
>
> So either I need a goto at the end of the first if statement before the closing 
> brace, or I need to adjust some other things in the code.
>
> Any thoughts from anyone on the proper way to approach this ;). I'd like to get 
> this one done before try 10 (haha).
>
> -Zac

Actually I guess just make it an if..else..if would work too.

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

[Zac Brown]

Michael Karcher wrote:
> Am Donnerstag, den 19.06.2008, 12:52 -0700 schrieb Zac Brown:
> > - Because these tests are mutually exclusive, a single goto label suffices for 
> > breaking the loop.
>
> As written, your tests are not mutually exclusive. It could happen that
> flags has IS_TEXT_UNICODE_CONTROLS and IS_TEXT_UNICODE_REVERSE_CONTROLS,
> set and out_flags contains IS_TEXT_UNICODE_REVERSE_SIGNATURE. (If the
> caller doesn't provide flags, both flags are set, in fact!) In that
> case, both conditions of your if statements are true.
>
> I don't know whether that means that you need two goto targets or that
> your logic should be changed, but I suspect the latter. If you test for
> reversed control characters only I a reversed BOM was found, you should
> not test for non-reversed control characters if a reversed BOM was
> found, according to my intuition.
>
> Regards,
>   Michael Karcher

After re-reading the documentation, it is unclear as to whether the tests are 
truly mutually exclusive. I took the mutually exclusive hint from comments in 
the code actually.

Does anyone have feedback on whether they believe the tests for normal control 
characters vs reversed control characters should be mutually exlusive? To me it 
makes sense for them to be, but that doesn't necessarily mean they should be...

Documentation for those interested: 
http://msdn.microsoft.com/en-us/library/ms776445.aspx

Comments are welcome and *greatly* appreciated.

-Zac

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

[Roy Shea]

Hello Zac,
> Does anyone have feedback on whether they believe the tests for normal
> control characters vs reversed control characters should be mutually
> exlusive?

I took a look at the documentation.  My read is that the tests are not
mutually exclusive.

Given your current version and my reading of the documentation you would
need two separate goto targets: control_char_done and
reverse_control_char_done.

Note that all of this is based on my reading of the documentation.
You'll need to dig into some Windows testing.  Off the top of my head I
would try a set of four tests:

Flags IS_TEXT_UNICODE_CONTROLS and IS_TEXT_UNICODE_REVERSE_CONTROLS set
and then pass in a string:

- with no controls
- with unicode control
- with reversed unicode control
- with both

This teases out if your IS_TEXT_UNICODE_CONTROLS needs to expand its
conditional with:

----
!(out_flags & IS_TEXT_UNICODE_REVERSE_SIGNATURE)
----

and provides evidence as to the mutual exclusivity of the two tests.
For full Windows compliance you could also test:

- only the IS_TEXT_UNICODE_CONTROLS flag and a string with reversed
  unicode controls (does it set the flag for reversed controls?)

- only the IS_TEXT_UNICODE_REVERSE_CONTROLS flag and a string with
  unicode controls (does it set the flag for controls?)


One additional point.  The documentation explicitly notes that:

"Only flags that are set upon input to the function are significant upon
output."

Based on the documentation, setting extra flags should not be a problem.
But for full conformance with Windows, you may not have this
flexibility.

Best of luck!
-Roy