New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Issue 54] Removed always-true if and unreachable code in XObject.cs #55

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@andi9310
Contributor

andi9310 commented Nov 13, 2014

Removed always-true if and unreachable code in XObject.cs

@andi9310 andi9310 changed the title from Issue 54 to [Issue 54] Removed always-true if and unreachable code in XObject.cs Nov 13, 2014

@davkean

This comment has been minimized.

Show comment
Hide comment
@davkean

davkean Nov 13, 2014

Thanks for the pull request. This particular code hasn't been touched for a while, so I'm a little rusty around it. Can you provide a little more information on why you think this is dead code? If I'm reading the new code correctly, it looks like this now returns false when we hit a parent with any annotations.

Thanks for the pull request. This particular code hasn't been touched for a while, so I'm a little rusty around it. Can you provide a little more information on why you think this is dead code? If I'm reading the new code correctly, it looks like this now returns false when we hit a parent with any annotations.

This comment has been minimized.

Show comment
Hide comment
@ranma42

ranma42 Nov 24, 2014

@davkean I'm afraid you are both right & wrong: the code returns false when it hits a parent with any annotation, not just with an XObjectChangeAnnotation, but this is the same behaviour as before.

From my reading of the code, o.Annotations< XObjectChangeAnnotation>() is always going to return a valid IEnumerable, so the issue could be that instead of o.Annotations<XObjectChangeAnnotation>() != null the second if should check for o.Annotations<XObjectChangeAnnotation>().Any()

@davkean I'm afraid you are both right & wrong: the code returns false when it hits a parent with any annotation, not just with an XObjectChangeAnnotation, but this is the same behaviour as before.

From my reading of the code, o.Annotations< XObjectChangeAnnotation>() is always going to return a valid IEnumerable, so the issue could be that instead of o.Annotations<XObjectChangeAnnotation>() != null the second if should check for o.Annotations<XObjectChangeAnnotation>().Any()

@andi9310

This comment has been minimized.

Show comment
Hide comment
@andi9310

andi9310 Nov 13, 2014

Contributor

@davkean , thanks for response. As of I understand this code, "IEnumerable Annotations() "method can return IEnumerable with contents, empty IEnumerable, but never null. So "o.Annotations() != null" will always be true. I don't know really if it is supposed to work this way. Maybe It should be "o.Annotations() .Any()" here? As far as i know yielding methods are meant to be used with foreach statements etc. so returning null from them would end with NullReferenceExceptions.

Contributor

andi9310 commented Nov 13, 2014

@davkean , thanks for response. As of I understand this code, "IEnumerable Annotations() "method can return IEnumerable with contents, empty IEnumerable, but never null. So "o.Annotations() != null" will always be true. I don't know really if it is supposed to work this way. Maybe It should be "o.Annotations() .Any()" here? As far as i know yielding methods are meant to be used with foreach statements etc. so returning null from them would end with NullReferenceExceptions.

@davkean

This comment has been minimized.

Show comment
Hide comment
@davkean

davkean Nov 13, 2014

Member

Yep, you are correct that o.Annotations<XObjectChangeAnnotation>() != null will never return null. Looks like they were really supposed to use the non-plural version; o.Annotation<XObjectChangeAnnotation> != null.

However, before we remove or change the check - this particular code was written ~8 years ago, I'd like to get an understanding of the behavior that is broken by that bad check - ie some test cases or code samples that show that fixing this gives a better, but still compatible behavior. For example, are we now firing Changing/Changed events when we're not supposed to?

Member

davkean commented Nov 13, 2014

Yep, you are correct that o.Annotations<XObjectChangeAnnotation>() != null will never return null. Looks like they were really supposed to use the non-plural version; o.Annotation<XObjectChangeAnnotation> != null.

However, before we remove or change the check - this particular code was written ~8 years ago, I'd like to get an understanding of the behavior that is broken by that bad check - ie some test cases or code samples that show that fixing this gives a better, but still compatible behavior. For example, are we now firing Changing/Changed events when we're not supposed to?

@davkean

This comment has been minimized.

Show comment
Hide comment
@davkean

davkean Nov 22, 2014

Member

We appreciate the pull request, however, we've very wary about changing/removing code without tests to prove that an issue exists. Feel free to submit another pull request when you have coverage that covers and proves the bug.

Member

davkean commented Nov 22, 2014

We appreciate the pull request, however, we've very wary about changing/removing code without tests to prove that an issue exists. Feel free to submit another pull request when you have coverage that covers and proves the bug.

@davkean davkean closed this Nov 22, 2014

@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016

luhenry pushed a commit to luhenry/corefx that referenced this pull request May 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment