MSBuild.Engine: use a buildtask enumerator that throws exceptions #444

Merged
merged 3 commits into from Sep 3, 2012

Conversation

Projects
None yet
2 participants
@knocte
Contributor

knocte commented Aug 22, 2012

InvalidOperationExceptions were not being thrown when asking for the
Current element if the enumerator had finished (or not started)
iterating.

MSBuild.Engine: use a buildtask enumerator that throws exceptions
InvalidOperationExceptions were not being thrown when asking for the
Current element if the enumerator had finished (or not started)
iterating.
@knocte

This comment has been minimized.

Show comment Hide comment
@knocte

knocte Sep 3, 2012

Contributor

Ankit, oh I think I realised what you were referring to. But the answer is no, there's no yield vs GetEnumerator difference here.

To demonstrate, I've run this TestFixture in both Mono and MS.NET and it passes in both:

[TestFixture]
public class MainClass
{
    private List<string> listOfTwoElements = new List<string> {"one", "two"};

    private IEnumerator YieldAsEnumerator ()
    {
        foreach(var element in listOfTwoElements)
            yield return element;
    }

    [Test]
    public void YieldActingAsIEnumerator ()
    {
        var enumerator = YieldAsEnumerator();
        var current = enumerator.Current;
        Assert.That(current, Is.Null);
        Assert.That(enumerator.MoveNext(), Is.EqualTo(true)); 
        current = enumerator.Current;
        Assert.That(current, Is.EqualTo("one"));
        Assert.That(enumerator.MoveNext(), Is.EqualTo(true));
        current = enumerator.Current;
        Assert.That(current, Is.EqualTo("two"));
        Assert.That(enumerator.MoveNext(), Is.EqualTo(false));
        current = enumerator.Current;
        Assert.That(current, Is.EqualTo("two"));
    }

}

So then, the usage of a normal yield in MSBuild is not right. The simple implementation of the enumerator of an array brings the enough checks for it to behave like MS.NET, so I think this patch is still right.

Contributor

knocte commented Sep 3, 2012

Ankit, oh I think I realised what you were referring to. But the answer is no, there's no yield vs GetEnumerator difference here.

To demonstrate, I've run this TestFixture in both Mono and MS.NET and it passes in both:

[TestFixture]
public class MainClass
{
    private List<string> listOfTwoElements = new List<string> {"one", "two"};

    private IEnumerator YieldAsEnumerator ()
    {
        foreach(var element in listOfTwoElements)
            yield return element;
    }

    [Test]
    public void YieldActingAsIEnumerator ()
    {
        var enumerator = YieldAsEnumerator();
        var current = enumerator.Current;
        Assert.That(current, Is.Null);
        Assert.That(enumerator.MoveNext(), Is.EqualTo(true)); 
        current = enumerator.Current;
        Assert.That(current, Is.EqualTo("one"));
        Assert.That(enumerator.MoveNext(), Is.EqualTo(true));
        current = enumerator.Current;
        Assert.That(current, Is.EqualTo("two"));
        Assert.That(enumerator.MoveNext(), Is.EqualTo(false));
        current = enumerator.Current;
        Assert.That(current, Is.EqualTo("two"));
    }

}

So then, the usage of a normal yield in MSBuild is not right. The simple implementation of the enumerator of an array brings the enough checks for it to behave like MS.NET, so I think this patch is still right.

@radical

View changes

...ss/Microsoft.Build.Engine/Test/Microsoft.Build.BuildEngine/TargetTest.cs
+
+ bool thrown = false;
+ try
+ {

This comment has been minimized.

Show comment Hide comment
@radical

radical Sep 3, 2012

Member

try {
..
} catch {
..
}

@radical

radical Sep 3, 2012

Member

try {
..
} catch {
..
}

+ Assert.AreEqual (true, e.MoveNext (), "A5");
+ Assert.AreEqual ("Message", ((BuildTask)e.Current).Name, "A6");
+ Assert.AreEqual (false, e.MoveNext (), "A7");
+ try

This comment has been minimized.

Show comment Hide comment
@radical

radical Sep 3, 2012

Member

Same here

@radical

radical Sep 3, 2012

Member

Same here

@knocte

This comment has been minimized.

Show comment Hide comment
@knocte

knocte Sep 3, 2012

Contributor

Fixed coding style in both try-catch blocks.

Contributor

knocte commented Sep 3, 2012

Fixed coding style in both try-catch blocks.

radical added a commit that referenced this pull request Sep 3, 2012

Merge pull request #444 from knocte/xbuild_improvements
MSBuild.Engine: use a buildtask enumerator that throws exceptions

@radical radical merged commit 1113782 into mono:master Sep 3, 2012

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