Skip to content
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

Fix byref parameter #204

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@fabienDaou
Copy link

commented Nov 12, 2018

Fixes #264

Problem

There is an issue with ByRef parameter that accept literal values when the method is invoked.

DoSomething(ref "sometext") is not supported in C# but it does not complain in VB (at least in my project.)

Solution

For every literal values, I declared a local variable and used it instead.

Hope it helps,

Fabien

PS: I am not familiar with the framework to do code analyses so bear with me while I learn :)

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

Thanks for the PR, you're definitely thinking along the right lines.

In general, the rest of the code in the two main visitor classes avoid post-processing. I've added a point to the contributing doc to help others in future avoid this pattern hopefully.

Before diving into changing that though, there's something else that I think's worth some thought. I notice that this approach won't work for ref literal in a field declaration, but whatever approach is used for a literal in a field declaration will probably work for this too. If that case seems too complex, I'd still merge a solution just to this problem within methods, since it makes things better, but let's throw out some ideas first.

e.g. Should we create a new method to contain the extra variable declarations, then call it with inline declarations as shown here where DoSomethingOut1 is generated and DoSomethingElse is the original callsite

    public void DoSomethingElse()
    {
        DoSomethingOut1(out var val0, out var val1, out var dble, "sometext2");
    }

    public void DoSomethingOut1(out string str, out bool val, out double dble, string str2)
    {
        dble = 2;
        val0 = "sometext";
        val1 = true;
        DoSomething(ref val0, ref val1, ref dble, str2);
    }

I think I'm right in saying this will work for all cases. In terms of implementation, we'd keep a list of these extra method declarations - see NodesVisitor.AdditionalStatements, and add them at the end, meaning the rest of the transformation could be done directly.

Let me know if you're interested in having a try at implementing this, or have another simpler idea.

Also I'm very happy to take feedback or contributions to help others get started by the way, I know it can be daunting.

Thanks again!

@fabienDaou

This comment has been minimized.

Copy link
Author

commented Nov 15, 2018

I will change the code to avoid post-processing as suggested.

Just to be sure I understand. By "ref literal in a field declaration", do you mean this:

Class Surrounding

    Public Str As String = DoStuff("bla")

    Private Shared Function DoStuff(ByRef str As String) As String
        str = "DoStuff"
        Return str
    End Function

End Class

By the way, I am not going to be able to contribute for the next two weeks so it might take some times before I make the changes.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

Yep that's the one. OK thanks for the heads up - I'll leave this open. Remember to branch from, merge or rebase onto master when you continue, and feel free to totally nuke my commits if they're no longer relevant of course.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

Are you still interested in continuing this @fabienDaou ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.