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

Allow to assign null AST childs after non-null #259

Merged
merged 1 commit into from Jun 24, 2015

Conversation

Projects
None yet
3 participants
@leventov
Contributor

leventov commented Jun 15, 2015

See #249

@GerardPaligot

This comment has been minimized.

Show comment
Hide comment
@GerardPaligot

GerardPaligot Jun 17, 2015

Contributor

This feature seems necessary. We shouldn't have a NPE when we set a target with null. But my first reaction was that it is awful to check everywhere if the parameter is null. After reflection, if we want to keep the setters of parents and/or target on the AST, we don't have any choices. So, I'm ok with this PR.

Contributor

GerardPaligot commented Jun 17, 2015

This feature seems necessary. We shouldn't have a NPE when we set a target with null. But my first reaction was that it is awful to check everywhere if the parameter is null. After reflection, if we want to keep the setters of parents and/or target on the AST, we don't have any choices. So, I'm ok with this PR.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov
Contributor

leventov commented Jun 17, 2015

@@ -46,7 +46,8 @@ public void accept(CtVisitor visitor) {
}
public void setExpression(CtExpression<T> value) {
value.setParent(this);
if (value != null)
value.setParent(this);

This comment has been minimized.

@GerardPaligot

GerardPaligot Jun 18, 2015

Contributor

An expression of an assertion can be null? For me, an assertion must have an expression so I propose to change for something like:

public void setExpression(CtExpression<T> value) {
    if (value == null) {
        throw new SpoonException("An assertion must have an expression.");
    }
    value.setParent(this);
    this.value = value;
}

What do you think?

@GerardPaligot

GerardPaligot Jun 18, 2015

Contributor

An expression of an assertion can be null? For me, an assertion must have an expression so I propose to change for something like:

public void setExpression(CtExpression<T> value) {
    if (value == null) {
        throw new SpoonException("An assertion must have an expression.");
    }
    value.setParent(this);
    this.value = value;
}

What do you think?

This comment has been minimized.

@leventov

leventov Jun 18, 2015

Contributor

There is an "assertionExpression" and just "expression". Null expression corresponds to the statement assert assertionExpression;. Or it might be assert assertionExpression : expression;

@leventov

leventov Jun 18, 2015

Contributor

There is an "assertionExpression" and just "expression". Null expression corresponds to the statement assert assertionExpression;. Or it might be assert assertionExpression : expression;

@GerardPaligot

This comment has been minimized.

Show comment
Hide comment
@GerardPaligot

GerardPaligot Jun 18, 2015

Contributor

For now, you have only one test in your PR to test a target null but there aren't tests for other checks. It could be interesting to add new tests for all others cases. We know now why we add these check but not necessarily in a few years.

So, can you add tests?

Contributor

GerardPaligot commented Jun 18, 2015

For now, you have only one test in your PR to test a target null but there aren't tests for other checks. It could be interesting to add new tests for all others cases. We know now why we add these check but not necessarily in a few years.

So, can you add tests?

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Jun 18, 2015

Contributor

Ok

Contributor

leventov commented Jun 18, 2015

Ok

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Jun 19, 2015

Contributor

@GerardPaligot please look why CI build is failing. I don't understand. Locally it is ok.

Contributor

leventov commented Jun 19, 2015

@GerardPaligot please look why CI build is failing. I don't understand. Locally it is ok.

monperrus added a commit that referenced this pull request Jun 24, 2015

@monperrus monperrus merged commit 4201453 into INRIA:master Jun 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment