Skip to content

Remove IronPython.Compiler.Ast.Arg#1175

Merged
slozier merged 4 commits intoIronLanguages:masterfrom
slozier:arg2
Apr 18, 2021
Merged

Remove IronPython.Compiler.Ast.Arg#1175
slozier merged 4 commits intoIronLanguages:masterfrom
slozier:arg2

Conversation

@slozier
Copy link
Copy Markdown
Contributor

@slozier slozier commented Apr 17, 2021

Gets rid of the annoying (to me) Arg construct. 😄

@BCSharp if you have any feedback. Not sure if this will creates merge conflicts with your PR.

@slozier slozier marked this pull request as draft April 17, 2021 22:39
@slozier
Copy link
Copy Markdown
Contributor Author

slozier commented Apr 17, 2021

Looks like there are some issues with the start/end indices during parsing.

@slozier
Copy link
Copy Markdown
Contributor Author

slozier commented Apr 18, 2021

Looks like I also broke:

class test(*[object]): pass

@BCSharp
Copy link
Copy Markdown
Member

BCSharp commented Apr 18, 2021

@BCSharp if you have any feedback.

I don't care about Arg that much. There are other things that bug me way more.

Not sure if this will creates merge conflicts with your PR.

I does create a merge conflict in CallExpression that is easilly fixable. More importantly, however, the code will not compile because I still use IReadOnlyList<Arg> Keywords in ClassDefinition.

@slozier slozier marked this pull request as ready for review April 18, 2021 17:48
@slozier slozier merged commit c6a4cb4 into IronLanguages:master Apr 18, 2021
@slozier slozier deleted the arg2 branch April 18, 2021 18:57
Comment thread Src/IronPython/Compiler/Ast/ClassDefinition.cs
Comment thread Src/IronPython/Compiler/Ast/CallExpression.cs
Comment thread Src/IronPython/Compiler/Ast/ClassDefinition.cs
Comment thread Src/IronPython/Compiler/Ast/CallExpression.cs
Comment thread Src/IronPython/Compiler/Ast/Keyword.cs
public IList<Expression> Items => _items;
public IReadOnlyList<Expression> Items => _items;

internal Expression[] UnsafeItems => _items; // TODO: get rid of this
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good luck with this! 😄 IReadOnlyCollection should have had CopyTo, just like ICollection has. I suppose ArrayUtils.Insert<T>(T item, IReadOnlyList<T> list) can also do if (list is T[] array) ... to maintain performance .

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.

Unfortunately ArrayUtils.Insert is in the DLR and I didn't want to rev it just for this. I guess as long as we don't start using UnsafeItems all over the place (I don't see why we would) this is fine.

@slozier
Copy link
Copy Markdown
Contributor Author

slozier commented Apr 18, 2021

@BCSharp Thanks for the review! All excellent points. I guess I was a bit quick on the merge. 😄

@slozier slozier mentioned this pull request Apr 18, 2021
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.

2 participants