Re: [try2] advapi32: Implement CredReadDomainCredentials stub and tests

[James Hawkins]

On Thu, Oct 9, 2008 at 4:14 PM, Andrey Turkin <andrey.turkin@...> wrote:

>
> [now with tests, fixed and reformatted as per James' suggestions]
>
> Implement CredReadDomainCredentialsA and CredReadDomainCredentialsW
> stubs and few tests for them.  Required for MSN Messenger 7.0
> ---
>  dlls/advapi32/advapi32.spec |    4 +-
>  dlls/advapi32/cred.c        |  160 +++++++++++++++++++++++++++++++++++++++++++
>  dlls/advapi32/tests/cred.c  |   45 ++++++++++++
>  include/wincred.h           |   39 +++++++++++
>  4 files changed, 246 insertions(+), 2 deletions(-)
>

+    *Size = 0;

+    *Credentials = NULL;

+    if (!TargetInformation)

What if Size or Credentials is NULL?  You should add a test for that
and handle it appropriately.  If the tests crash, you should comment
out the tests and note that there is no handling in Windows.

--
James Hawkins

[Andrey Turkin-3]

James Hawkins wrote:

On Thu, Oct 9, 2008 at 4:14 PM, Andrey Turkin andrey.turkin@... wrote:
  

[now with tests, fixed and reformatted as per James' suggestions]

Implement CredReadDomainCredentialsA and CredReadDomainCredentialsW
stubs and few tests for them.  Required for MSN Messenger 7.0
---
 dlls/advapi32/advapi32.spec |    4 +-
 dlls/advapi32/cred.c        |  160 +++++++++++++++++++++++++++++++++++++++++++
 dlls/advapi32/tests/cred.c  |   45 ++++++++++++
 include/wincred.h           |   39 +++++++++++
 4 files changed, 246 insertions(+), 2 deletions(-)

    

+    *Size = 0;

+    *Credentials = NULL;

+    if (!TargetInformation)

What if Size or Credentials is NULL?  You should add a test for that
and handle it appropriately.  If the tests crash, you should comment
out the tests and note that there is no handling in Windows.

  

Why? Windows does the same thing, tests would crash. Do you really think somebody need to know this? Is there any application that depends on this behavior so these assignments must be commented to protect them from change? I can't see any value in commented out test or sort-of-meaningless code comment.

[James Hawkins]

On Fri, Oct 10, 2008 at 12:59 AM, Andrey Turkin <andrey.turkin@...> wrote:

> James Hawkins wrote:
>
> On Thu, Oct 9, 2008 at 4:14 PM, Andrey Turkin <andrey.turkin@...>
> wrote:
>
>
> [now with tests, fixed and reformatted as per James' suggestions]
>
> Implement CredReadDomainCredentialsA and CredReadDomainCredentialsW
> stubs and few tests for them.  Required for MSN Messenger 7.0
> ---
>  dlls/advapi32/advapi32.spec |    4 +-
>  dlls/advapi32/cred.c        |  160
> +++++++++++++++++++++++++++++++++++++++++++
>  dlls/advapi32/tests/cred.c  |   45 ++++++++++++
>  include/wincred.h           |   39 +++++++++++
>  4 files changed, 246 insertions(+), 2 deletions(-)
>
>
>
> +    *Size = 0;
>
> +    *Credentials = NULL;
>
> +    if (!TargetInformation)
>
> What if Size or Credentials is NULL?  You should add a test for that
> and handle it appropriately.  If the tests crash, you should comment
> out the tests and note that there is no handling in Windows.
>
>
>
> Why? Windows does the same thing, tests would crash. Do you really think
> somebody need to know this? Is there any application that depends on this
> behavior so these assignments must be commented to protect them from change?
> I can't see any value in commented out test or sort-of-meaningless code
> comment.
>

The tests serve as documentation of the API.  In many cases, that
documentation is far superior to even msdn.  Just because you know
that the implementation matches native doesn't mean that someone else
looking to work on the function knows that.  He'll then waste time
figuring out what you already know.  Ask yourself the opposite: what's
the harm in adding such documentation?

--
James Hawkins

[Andrey Turkin-3]

James Hawkins wrote:

On Fri, Oct 10, 2008 at 12:59 AM, Andrey Turkin andrey.turkin@... wrote:
  

I can't see any value in commented out test or sort-of-meaningless code
comment.

    

The tests serve as documentation of the API.  In many cases, that
documentation is far superior to even msdn.  Just because you know
that the implementation matches native doesn't mean that someone else
looking to work on the function knows that.  He'll then waste time
figuring out what you already know.  Ask yourself the opposite: what's
the harm in adding such documentation?
  

Non-misleading comment obviously cannot make any harm, and I can imagine one (unlikely but not impossible)
case when comment can be helpful so yes, I'll add both commented out tests and sort-of-meaningless comment :)
Hopefully nobody will object them.

Regards,
  Andrey

[James Hawkins]

On Fri, Oct 10, 2008 at 2:51 PM, Andrey Turkin <andrey.turkin@...> wrote:

> James Hawkins wrote:
>
> On Fri, Oct 10, 2008 at 12:59 AM, Andrey Turkin <andrey.turkin@...>
> wrote:
>
>
> I can't see any value in commented out test or sort-of-meaningless code
> comment.
>
>
>
> The tests serve as documentation of the API.  In many cases, that
> documentation is far superior to even msdn.  Just because you know
> that the implementation matches native doesn't mean that someone else
> looking to work on the function knows that.  He'll then waste time
> figuring out what you already know.  Ask yourself the opposite: what's
> the harm in adding such documentation?
>
>
> Non-misleading comment obviously cannot make any harm, and I can imagine one
> (unlikely but not impossible)
> case when comment can be helpful so yes, I'll add both commented out tests
> and sort-of-meaningless comment :)
> Hopefully nobody will object them.
>

Please don't add a comment to the implementation.  The tests are
documentation enough.  All you need to do is if(0) out the tests that
crash.

--
James Hawkins

[Andrey Turkin-3]

James Hawkins wrote:

On Fri, Oct 10, 2008 at 2:51 PM, Andrey Turkin andrey.turkin@... wrote:
  

James Hawkins wrote:

On Fri, Oct 10, 2008 at 12:59 AM, Andrey Turkin andrey.turkin@...
wrote:


I can't see any value in commented out test or sort-of-meaningless code
comment.



The tests serve as documentation of the API.  In many cases, that
documentation is far superior to even msdn.  Just because you know
that the implementation matches native doesn't mean that someone else
looking to work on the function knows that.  He'll then waste time
figuring out what you already know.  Ask yourself the opposite: what's
the harm in adding such documentation?


Non-misleading comment obviously cannot make any harm, and I can imagine one
(unlikely but not impossible)
case when comment can be helpful so yes, I'll add both commented out tests
and sort-of-meaningless comment :)
Hopefully nobody will object them.

    

Please don't add a comment to the implementation.  The tests are
documentation enough.  All you need to do is if(0) out the tests that
crash.

  

Imagine broken application which for some reason, e.g. non-allocated memory, supply NULL to this function, and then catch, eat and spew an exception somewhere in different place. Joe the Developer start searching for root cause - he looks at logs, see first-chance exception, look in CredReadDomainCredentials source and see unguarded dereferencing. Is this intended or mistake? That should code really do? The comment would be handy here (at least for me tests wouldn't be the first place to look at for answers in such situation).
I can see the value and cannot see any harm, and frankly I am reluctant to issuing [try4] patch with just another minor comment change.

[James Hawkins]

On Fri, Oct 10, 2008 at 3:17 PM, Andrey Turkin <andrey.turkin@...> wrote:

>
> Imagine broken application which for some reason, e.g. non-allocated memory,
> supply NULL to this function, and then catch, eat and spew an exception
> somewhere in different place. Joe the Developer start searching for root
> cause - he looks at logs, see first-chance exception, look in
> CredReadDomainCredentials source and see unguarded dereferencing. Is this
> intended or mistake? That should code really do? The comment would be handy
> here (at least for me tests wouldn't be the first place to look at for
> answers in such situation).
> I can see the value and cannot see any harm, and frankly I am reluctant to
> issuing [try4] patch with just another minor comment change.
>

You just said that the tests crash in Windows.  If the tests crash in
Windows, then how does this app work there either?  That's the point
of tests: to verify that our implementation matches native.

--
James Hawkins

[Andrey Turkin-3]

James Hawkins wrote:

On Fri, Oct 10, 2008 at 3:17 PM, Andrey Turkin andrey.turkin@... wrote:
  

Imagine broken application which for some reason, e.g. non-allocated memory,
supply NULL to this function, and then catch, eat and spew an exception
somewhere in different place. Joe the Developer start searching for root
cause - he looks at logs, see first-chance exception, look in
CredReadDomainCredentials source and see unguarded dereferencing. Is this
intended or mistake? That should code really do? The comment would be handy
here (at least for me tests wouldn't be the first place to look at for
answers in such situation).
I can see the value and cannot see any harm, and frankly I am reluctant to
issuing [try4] patch with just another minor comment change.

    

You just said that the tests crash in Windows.  If the tests crash in
Windows, then how does this app work there either?  That's the point
of tests: to verify that our implementation matches native.
  

We know my implementation and Windows implementation behave identically and this is documented in tests. At this
point you and me both happy about the comment in tests and tests in general, right?
Now back to the example. In this example something somewhere went wrong because of Windows/Wine differences,
 e.g. previous memory allocation failed or something. Completely unrelated to this particular function, and our Joe should
not spend his time trying to understand the function itself - and code comments do just that. Joe read the comment, understand
that NULL pointer should not be passed in the first place, and continue his quest. Sounds useful for me, and certainly looks
harmless (apart from causing this thread to go on).

[James Hawkins]

On Fri, Oct 10, 2008 at 3:49 PM, Andrey Turkin <andrey.turkin@...> wrote:

>
> We know my implementation and Windows implementation behave identically and
> this is documented in tests. At this
> point you and me both happy about the comment in tests and tests in general,
> right?
> Now back to the example. In this example something somewhere went wrong
> because of Windows/Wine differences,
>  e.g. previous memory allocation failed or something. Completely unrelated
> to this particular function, and our Joe should
> not spend his time trying to understand the function itself - and code
> comments do just that. Joe read the comment, understand
> that NULL pointer should not be passed in the first place, and continue his
> quest. Sounds useful for me, and certainly looks
> harmless (apart from causing this thread to go on).
>

If you want it in, then leave it in.  That said, I think you're
discounting the usefulness of the tests.  In this situation, the first
thing I would do is look at the tests and see whether this behavior
has been tested and what the results are.  If it hasn't been tested,
then my first step towards fixing the bug is to add the appropriate
tests.  Maybe we have different philosophies on comments; I don't like
to comment things are obvious, and the tests make this point obvious.

--
James Hawkins

[James Mckenzie]

James Hawkins wrote:

> On Fri, Oct 10, 2008 at 3:49 PM, Andrey Turkin <andrey.turkin@...> wrote:
>  
>> We know my implementation and Windows implementation behave identically and
>> this is documented in tests. At this
>> point you and me both happy about the comment in tests and tests in general,
>> right?
>> Now back to the example. In this example something somewhere went wrong
>> because of Windows/Wine differences,
>>  e.g. previous memory allocation failed or something. Completely unrelated
>> to this particular function, and our Joe should
>> not spend his time trying to understand the function itself - and code
>> comments do just that. Joe read the comment, understand
>> that NULL pointer should not be passed in the first place, and continue his
>> quest. Sounds useful for me, and certainly looks
>> harmless (apart from causing this thread to go on).
>>
>>    
>
> If you want it in, then leave it in.  That said, I think you're
> discounting the usefulness of the tests.  In this situation, the first
> thing I would do is look at the tests and see whether this behavior
> has been tested and what the results are.  If it hasn't been tested,
> then my first step towards fixing the bug is to add the appropriate
> tests.  Maybe we have different philosophies on comments; I don't like
> to comment things are obvious, and the tests make this point obvious.
>
>  

James and Andrey:

I'm of the feeling the more comments the better and better documentation
is a 'good thing'.

If Andrey wants to put comments in, leave them there.  Maybe someone who
has no idea what is going on will read his comments and figure out a
better way for testing.

James McKenzie