Skip to content

enable DefaultArguments functions with mismatch signature - #4518

Merged
ke-yu merged 5 commits into
DynamoDS:masterfrom
mnhng:defaultArgs
May 25, 2015
Merged

ke-yu merged 5 commits into
DynamoDS:masterfrom
mnhng:defaultArgs

Conversation

@mnhng

@mnhng mnhng commented May 20, 2015

Copy link
Copy Markdown
Contributor

Purpose

  • For functions with added arguments that have default values, set UsingDefaultValue attribute to true.
  • This is done after Deserialize() call because for new arguments, the PortInfo tags aren't there in the saved dyn file so the UsingDefaultValue is set to false. Putting the signature check after Deserialize will have no effect.
  • In case the function to be migrated has different signature, the FunctionDescriptor should be the one that best matches the saved node, not the first in the list. (e.g Foo@int,int should be migrated to Foo@int,int,bool instead of Foo@double,double).

Reviewers

@ke-yu PTAL

FYIs

@aparajit-pratap

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here i no need to be increased? I'd suggest to cleanup this foreach to make it more readable, for example,

int i = 0;
foreach (var param in descriptor.Parameters)
{
    i++;
    ...
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we compare the type, and if type mismatch, then may choose using default argument. What happen if a function is overloaded? For example,

Foo(Point p1 = default point 2, Point p2 = default point 1);
Foo(Line l1, Line l2);

And we open a .dyn whose signature is Foo@Line,Line, will it be changed to Foo@Point,Point and use default argument?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case the new parameter is added in the beginning, for example
old param: double
new param: Point,double
then when comparing Point and double, we shouldn't increase i

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the signature in the file is Foo@Line,Line then the descriptor MangledName is also Foo@Line,Line so the condition is evaluated as false and this step is skipped. The function will still be Foo@Line,Line not Foo@Point,Point.

@mnhng mnhng changed the title enable DefaultArguments functions with mismatch signature DMN enable DefaultArguments functions with mismatch signature May 20, 2015
@aparajit-pratap

Copy link
Copy Markdown
Contributor

Thanks @nguyen-binh-minh, I have tested your code. It works with my new Excel node.

@mnhng mnhng changed the title DMN enable DefaultArguments functions with mismatch signature enable DefaultArguments functions with mismatch signature May 21, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@ke-yu

ke-yu commented May 22, 2015

Copy link
Copy Markdown
Contributor

Please add a test cases for foo@int -> foo@int, double=default_value,double=default_value.

LGTM. Thanks!

@mnhng

mnhng commented May 22, 2015

Copy link
Copy Markdown
Contributor Author

updated with 2 more tests

@ke-yu ke-yu added the LGTM label May 22, 2015
ke-yu added a commit that referenced this pull request May 25, 2015
enable DefaultArguments functions with mismatch signature
@ke-yu
ke-yu merged commit 6663b50 into DynamoDS:master May 25, 2015
@mnhng
mnhng deleted the defaultArgs branch July 21, 2015 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants