Skip to content
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

fix: Remove parentheses around targeted expr in binary or unary op #3024

Merged
merged 3 commits into from
Jun 19, 2019

Conversation

Egor18
Copy link
Contributor

@Egor18 Egor18 commented Jun 18, 2019

fix #3021
I think there is no need to wrap any CtTargetedExpression in parentheses when it is used in binary / unary operations (precedence is higher anyway).

@Egor18 Egor18 changed the title Remove parentheses around targeted expr in binary or unary op fix: Remove parentheses around targeted expr in binary or unary op Jun 18, 2019
@tdurieux
Copy link
Collaborator

I think it can be required when the element is casted.

@Egor18
Copy link
Contributor Author

Egor18 commented Jun 19, 2019

The case with casts is still handled:

private boolean shouldSetBracket(CtExpression<?> e) {
if (!e.getTypeCasts().isEmpty()) {
return true;
}

I added a test just in case.

@monperrus
Copy link
Collaborator

LGTM.

we may miss a special case here, but if there is no test, we don't know. We will catch it with jenkins and upcoming #2724

@Egor18
Copy link
Contributor Author

Egor18 commented Jun 19, 2019

Well, I looked through all of the possible CtTargetedExpressions:
CtAnnotationFieldAccess<T>, CtArrayAccess<T,E>, CtArrayRead<T>, CtArrayWrite<T>, CtConstructorCall<T>, CtExecutableReferenceExpression<T,E>, CtFieldAccess<T>, CtFieldRead<T>, CtFieldWrite<T>, CtInvocation<T>, CtNewClass<T>, CtSuperAccess<T>, CtThisAccess<T>
and all possible binary and unary operators, and I don't see any counterexamples.

BTW, the similar fix is possible for #2330.

@nharrand
Copy link
Collaborator

LGTM too.

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.

4 participants