Quantcast

[Zbarw-devel] assert(rc == WAIT_OBJECT_0 || rc == WAIT_FAILED)

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Zbarw-devel] assert(rc == WAIT_OBJECT_0 || rc == WAIT_FAILED)

jarekczek
Administrator
+        // error handling
          assert(rc == WAIT_OBJECT_0  ||  rc == WAIT_FAILED);

Klaus, why do you need that assertion, while all error conditions are
reported appropriately?

// error handling
seems to be an example of overcommenting. Anyone sees that this is error
handling, so having another line explaining it means that 1 line of code
must be invisible, because this comment takes its place.

So there are 2 commits fixing 1 bug and the result is still not the
best. That's what I wanted to avoid and stopped after reporting the bug.
There is actually no problem at all, but please note that coding in
public repository under time pressure does not give best results :)

Jarek

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Zbarw-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/zbarw-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Zbarw-devel] assert(rc == WAIT_OBJECT_0 || rc == WAIT_FAILED)

klaus triendl
Am Wed, 13 Jun 2012 08:48:27 +0200
schrieb Jarek Czekalski <[hidden email]>:

> +        // error handling
>           assert(rc == WAIT_OBJECT_0  ||  rc == WAIT_FAILED);
>
> Klaus, why do you need that assertion, while all error conditions are
> reported appropriately?

Well, the idea was to handle WAIT_ABANDONED as success, but assert in
debug mode that it doesn't happen (programming error). It's indeed
superfluous.


> // error handling
> seems to be an example of overcommenting. Anyone sees that this is
> error handling, so having another line explaining it means that 1
> line of code must be invisible, because this comment takes its place.

I agree in terms of perfection, but it sounds like a little
overreaction... For me it visibly marks a code block even it doesn't
enlighten anybody.


> So there are 2 commits fixing 1 bug and the result is still not the
> best. That's what I wanted to avoid and stopped after reporting the
> bug. There is actually no problem at all, but please note that coding
> in public repository under time pressure does not give best results :)

Well the nature of the bug is very clear and it's fixed, which is the
most important. So what did you want to avoid? I shouldn't have changed
the error handling along with the actual bugfix, that's my real fault.

On the other hand I really don't care about a small inconsistenty in
the code history. I've invested so much time developing the directshow
driver, testing and finding bugs, that this is really peanuts compared
to it. If I would have understood things better before releasing the
initial directshow implementation this bug wouldn't have even existed.
Coding is always a work in progress.



Having defended myself I do agree with you about the goal to writing
clean code and creating concise changesets.



Klaus

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Zbarw-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/zbarw-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Zbarw-devel] assert(rc == WAIT_OBJECT_0 || rc == WAIT_FAILED)

jarekczek
Administrator


W dniu 2012-06-13 20:57, klaus triendl pisze:
>
>> // error handling
>> seems to be an example of overcommenting. Anyone sees that this is
>> error handling, so having another line explaining it means that 1
>> line of code must be invisible, because this comment takes its place.
> I agree in terms of perfection, but it sounds like a little
> overreaction... For me it visibly marks a code block even it doesn't
> enlighten anybody.
My remark regarding the comment was only because it accompanies a line
that you agreed is redundant: "assert". So a single commit will remove both.
>
>> So there are 2 commits fixing 1 bug and the result is still not the
>> best. That's what I wanted to avoid and stopped after reporting the
>> bug. There is actually no problem at all, but please note that coding
>> in public repository under time pressure does not give best results :)
> Well the nature of the bug is very clear and it's fixed, which is the
> most important. So what did you want to avoid? I shouldn't have changed
> the error handling along with the actual bugfix, that's my real fault.
If this error handling was clean and readable, I wouldn't say a word.
But in open source project it's very important to make code easy to
understand, as it will be analyzed by hundreds of people.

> On the other hand I really don't care about a small inconsistenty in
> the code history. I've invested so much time developing the directshow
> driver, testing and finding bugs, that this is really peanuts compared
> to it. If I would have understood things better before releasing the
> initial directshow implementation this bug wouldn't have even existed.
You seem not to appreciate the work that was needed to reproduce and
isolate the error. This work includes understanding things better, which
resulted in code comments that were missing. After all it seems so easy...

> (...)
>
> Having defended myself I do agree with you about the goal to writing
> clean code and creating concise changesets.
Cool :)

Next days I'm going to work on the things that accumulated on my side. I
guess we have the hardest part behind us and our first release is to
come soon.

Jarek


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Zbarw-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/zbarw-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Zbarw-devel] assert(rc == WAIT_OBJECT_0 || rc == WAIT_FAILED)

klaus triendl
Am Thu, 14 Jun 2012 08:31:45 +0200
schrieb Jarek Czekalski <[hidden email]>:

>
>
> W dniu 2012-06-13 20:57, klaus triendl pisze:
> >
> >> // error handling
> >> seems to be an example of overcommenting. Anyone sees that this is
> >> error handling, so having another line explaining it means that 1
> >> line of code must be invisible, because this comment takes its
> >> place.
> > I agree in terms of perfection, but it sounds like a little
> > overreaction... For me it visibly marks a code block even it doesn't
> > enlighten anybody.
> My remark regarding the comment was only because it accompanies a
> line that you agreed is redundant: "assert". So a single commit will
> remove both.

Any other things before I commit?


> > On the other hand I really don't care about a small inconsistenty in
> > the code history. I've invested so much time developing the
> > directshow driver, testing and finding bugs, that this is really
> > peanuts compared to it. If I would have understood things better
> > before releasing the initial directshow implementation this bug
> > wouldn't have even existed.
> You seem not to appreciate the work that was needed to reproduce and
> isolate the error. This work includes understanding things better,
> which resulted in code comments that were missing. After all it seems
> so easy...

I didn't intend to undermine your efforts, which I appreciate. This
discussion would never end and goes already too far. Let's concentrate
on the good things. Thanks for your feedback!


> Next days I'm going to work on the things that accumulated on my
> side. I guess we have the hardest part behind us and our first
> release is to come soon.

Will look forward.



Klaus

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Zbarw-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/zbarw-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Zbarw-devel] assert(rc == WAIT_OBJECT_0 || rc == WAIT_FAILED)

jarekczek
Administrator
W dniu 2012-06-14 17:26, klaus triendl pisze:
>
> Any other things before I commit?

Let's finish it, together with vfw.c.

Jarek

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Zbarw-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/zbarw-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Zbarw-devel] assert(rc == WAIT_OBJECT_0 || rc == WAIT_FAILED)

klaus triendl
Am Thu, 14 Jun 2012 18:19:52 +0200
schrieb Jarek Czekalski <[hidden email]>:

> W dniu 2012-06-14 17:26, klaus triendl pisze:
> >
> > Any other things before I commit?
>
> Let's finish it, together with vfw.c.

See attached patch.

Notes:
* I've removed the `if (img)` when setting state->image=NULL because
  it's actually an unnecessary condition
* The wording in the comment "If available, this field is nulled."
  because the value of state->image will always be NULL upon leaving
  the function



Klaus
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Zbarw-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/zbarw-devel

vfw_dq_fix.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Zbarw-devel] assert(rc == WAIT_OBJECT_0 || rc == WAIT_FAILED)

jarekczek
Administrator

W dniu 2012-06-14 22:27, klaus triendl pisze:
>> Let's finish it, together with vfw.c.
> See attached patch.
>
> Notes:
> * I've removed the `if (img)` when setting state->image=NULL because
>    it's actually an unnecessary condition
> * The wording in the comment "If available, this field is nulled."
>    because the value of state->image will always be NULL upon leaving
>    the function

No objections, but please do a compile and run test.

Jarek


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Zbarw-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/zbarw-devel
Loading...