Do not export variables from comprehension cases in v3_core #269

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@nox
Contributor

nox commented Mar 5, 2014

Code like the following snippet could make the compiler crash:

f() -> [X = a || false] ++ [X = a || false].

@UlfNorell

@bjorng

This comment has been minimized.

Show comment
Hide comment
@bjorng

bjorng Mar 5, 2014

Contributor

Have you tested this code?

The logic of your correction seems to be backwards. You don't export any variables from a case except when it is a list comprehension.

Also, the compiler crashes when I attempt to build a new primary bootstrap or test suites.

Contributor

bjorng commented Mar 5, 2014

Have you tested this code?

The logic of your correction seems to be backwards. You don't export any variables from a case except when it is a list comprehension.

Also, the compiler crashes when I attempt to build a new primary bootstrap or test suites.

@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Mar 5, 2014

Contributor

Oh right, fixed something at the last moment and flipped the condition.

Contributor

nox commented Mar 5, 2014

Oh right, fixed something at the last moment and flipped the condition.

Do not export variables from comprehension cases in v3_core
Code like the following snippet could make the compiler crash:

    f() -> [X = a || false] ++ [X = a || false].

Reported-by: Ulf Norell
@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Mar 5, 2014

Contributor

Fixed and checked that the primary bootstrap can be built.

Contributor

nox commented Mar 5, 2014

Fixed and checked that the primary bootstrap can be built.

@bjorng

This comment has been minimized.

Show comment
Hide comment
@bjorng

bjorng Mar 5, 2014

Contributor

Looks good except that the lists: prefix should not be used in the compiler source code. I have fixed that and included the branch in our daily builds.

Contributor

bjorng commented Mar 5, 2014

Looks good except that the lists: prefix should not be used in the compiler source code. I have fixed that and included the branch in our daily builds.

@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Mar 5, 2014

Contributor

There are others list calls in the module, that's why I didn't import it.

Contributor

nox commented Mar 5, 2014

There are others list calls in the module, that's why I didn't import it.

@bjorng

This comment has been minimized.

Show comment
Hide comment
@bjorng

bjorng Mar 5, 2014

Contributor

Yes, I will talk to Björn-Egil about those other lists calls. :-)

There is no need import lists:member/2, since it is already imported.

Contributor

bjorng commented Mar 5, 2014

Yes, I will talk to Björn-Egil about those other lists calls. :-)

There is no need import lists:member/2, since it is already imported.

@psyeugenic

This comment has been minimized.

Show comment
Hide comment
@psyeugenic

psyeugenic Mar 5, 2014

Contributor

Kick him when he is down! =)

Contributor

psyeugenic commented Mar 5, 2014

Kick him when he is down! =)

@OTP-Maintainer

This comment has been minimized.

Show comment
Hide comment
@OTP-Maintainer

OTP-Maintainer Mar 5, 2014

Patch has passed first testings and has been assigned to be reviewed

Patch has passed first testings and has been assigned to be reviewed

@bjorng

This comment has been minimized.

Show comment
Hide comment
@bjorng

bjorng Mar 6, 2014

Contributor

Merged.

Contributor

bjorng commented Mar 6, 2014

Merged.

@bjorng bjorng closed this Mar 6, 2014

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