issue #32408 #65

Merged
merged 14 commits into from May 15, 2012

Conversation

Projects
None yet
2 participants
@paweljasinski
Contributor

paweljasinski commented Apr 18, 2012

I have fixed 32408 and did testing with test_ast.py
During testing I have discovered and fixed other things in _ast.cs:

  • Slices emit correct AST (no superflous Indexes added)
  • implemented pickle
  • Compare generates right AST for multiple operators (a<b<c)
  • added missing DictComp, SetComp and Set
  • difference between [:] and [::] is not lost
  • AST class hierarchy matches cpython (slice is no longer expr)
    Still missing is ast.parse support for DictComp, SetComp and Set

The submitted test_ast.py is modified. I have added some tests to help me with the issues.
There is a difference in col_offset for comprehensions which makes it unpractical to share test_ast.py between c and iron. There some other small iron specialties (break and continue must be in a loop, some parameters undergo stricter type checking).

I also believe that version of Python standard library which comes with HEAD is older than what came with 2.7.2.1. This causes test_increment_lineno to fail.

@jdhardy

This comment has been minimized.

Show comment Hide comment
@jdhardy

jdhardy Apr 19, 2012

Owner

Awesome work. I might not get a chance to review it until the weekend but it looks good. Just wanted to let you know it isn't being ignored.

Yes, the stdlib on master is behind ipy-27-maint. Eventually master will have the 3.2 stdlib but I haven't got around to it yet because it will be a huge change.

Owner

jdhardy commented Apr 19, 2012

Awesome work. I might not get a chance to review it until the weekend but it looks good. Just wanted to let you know it isn't being ignored.

Yes, the stdlib on master is behind ipy-27-maint. Eventually master will have the 3.2 stdlib but I haven't got around to it yet because it will be a huge change.

@jdhardy

This comment has been minimized.

Show comment Hide comment
@jdhardy

jdhardy May 8, 2012

Owner

Looks good to me. I'll pull it in before the next alpha release.

Owner

jdhardy commented May 8, 2012

Looks good to me. I'll pull it in before the next alpha release.

@jdhardy

This comment has been minimized.

Show comment Hide comment
@jdhardy

jdhardy May 13, 2012

Owner

I tried to pull it in but unfortunately it doesn't compile for WP7 or Android - their compilers don't understand the [Optional] declarations. You'll need to supply a constructor that does not have the [Optional] parameters.

For example,

public Break([Optional]int lineno, [Optional]int col_offset) {

would become

public Break() : this(null, null) { 

public Break([Optional]int? lineno, [Optional]int? col_offset) {

There's also a couple of places where you used int lineno instead of int? lineno.

Owner

jdhardy commented May 13, 2012

I tried to pull it in but unfortunately it doesn't compile for WP7 or Android - their compilers don't understand the [Optional] declarations. You'll need to supply a constructor that does not have the [Optional] parameters.

For example,

public Break([Optional]int lineno, [Optional]int col_offset) {

would become

public Break() : this(null, null) { 

public Break([Optional]int? lineno, [Optional]int? col_offset) {

There's also a couple of places where you used int lineno instead of int? lineno.

@paweljasinski

This comment has been minimized.

Show comment Hide comment
@paweljasinski

paweljasinski May 13, 2012

Contributor

Would it change the semantic from optional - "any combination allowed" to optional - "all or nothing"?
How about platform specific compilation?

I have copied the [Optional] use pattern from StringOps.cs
public static string encode(CodeContext/!/ context, string s, [Optional]object encoding, [DefaultParameterValue("strict")]string errors)
I suspect it changed since I branched, I will take a look how is it done now.

Contributor

paweljasinski commented May 13, 2012

Would it change the semantic from optional - "any combination allowed" to optional - "all or nothing"?
How about platform specific compilation?

I have copied the [Optional] use pattern from StringOps.cs
public static string encode(CodeContext/!/ context, string s, [Optional]object encoding, [DefaultParameterValue("strict")]string errors)
I suspect it changed since I branched, I will take a look how is it done now.

@jdhardy

This comment has been minimized.

Show comment Hide comment
@jdhardy

jdhardy May 13, 2012

Owner

Looking at it again, you do want to [Optional]s there, for IronPython. The problem is that the C# 4 compiler recognizes [Optional] and there are calls within the module that depend on that.

For example, you call Break() in a couple of places, which the C# 4 compiler can compile but the C# 3 compiler used for Android and WP7 can't - it wants all of the parameters.

I guess there are two options - always pass all of the parameters in the C# code, or add extra constructors for the older compilers without the [Optional] parameters and make them internal so that IronPython won't see them.

You'd then have

internal Break() : this(null, null) { 

which should preserve the [Optional] semantics for IronPython and make the older compilers happy.

Owner

jdhardy commented May 13, 2012

Looking at it again, you do want to [Optional]s there, for IronPython. The problem is that the C# 4 compiler recognizes [Optional] and there are calls within the module that depend on that.

For example, you call Break() in a couple of places, which the C# 4 compiler can compile but the C# 3 compiler used for Android and WP7 can't - it wants all of the parameters.

I guess there are two options - always pass all of the parameters in the C# code, or add extra constructors for the older compilers without the [Optional] parameters and make them internal so that IronPython won't see them.

You'd then have

internal Break() : this(null, null) { 

which should preserve the [Optional] semantics for IronPython and make the older compilers happy.

@paweljasinski

This comment has been minimized.

Show comment Hide comment
@paweljasinski

paweljasinski May 13, 2012

Contributor

ok, I am on it.

Contributor

paweljasinski commented May 13, 2012

ok, I am on it.

fixed compilation on WP7
added missing nullables
@paweljasinski

This comment has been minimized.

Show comment Hide comment
@paweljasinski

paweljasinski May 13, 2012

Contributor

changes as requested.
Compilation with WP7Debug passed.
I did not check compilation with Mono/Android.

Contributor

paweljasinski commented May 13, 2012

changes as requested.
Compilation with WP7Debug passed.
I did not check compilation with Mono/Android.

@jdhardy jdhardy merged commit f8d01c4 into IronLanguages:master May 15, 2012

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