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

feat: Add API method CtAbstractSwitch.addCaseAt #4015

Merged
merged 4 commits into from
Jul 2, 2021

Conversation

algomaster99
Copy link
Contributor

Reference: #3884 (comment).

@algomaster99
Copy link
Contributor Author

I had a few questions, a little unrelated to this PR, about two APIs in CtCase - setCaseExpression and addCaseExpression. What is the difference between their purposes? As far as I can see, the former modifies this.caseExpressions while the latter modifies this.caseExpression. Do we need to have the attribute this.caseExpression? I understand before Java 12 only one expression was allowed per case so maybe that's why this.caseExpression exists but now we can implement setCaseExpression to clear the list and just add one expression to the list of case expressions. Similary, getCaseExpression can just return the first element of the list.

I was using the setCaseExpression before and was trying to pretty-print CtCase after that but I did not see any expression in there because the visitor doesn't even look for this.caseExpression. The above suggestion can help resolve this.

@monperrus
Copy link
Collaborator

LGTM, will merge ;)

@algomaster99
Copy link
Contributor Author

@monperrus squash and merge it because some of the commits in between don't pass the CI tests so we don't want to break the master at any point. Moreover, the commit messages don't follow the spoon conventions too but the PR title does.

@slarse
Copy link
Collaborator

slarse commented Jun 29, 2021

@algomaster99 We always squash :)

@slarse
Copy link
Collaborator

slarse commented Jun 30, 2021

I understand before Java 12 only one expression was allowed per case so maybe that's why this.caseExpression exists but now we can implement setCaseExpression to clear the list and just add one expression to the list of case expressions.

Agree, open a separate issue/PR about that.

@monperrus monperrus changed the title feat: Add CtAbstractSwitch.addCaseAt feat: Add API method CtAbstractSwitch.addCaseAt Jul 2, 2021
@monperrus monperrus merged commit 66db36c into INRIA:master Jul 2, 2021
@monperrus
Copy link
Collaborator

Thanks a lot @algomaster99

@algomaster99 algomaster99 deleted the add-case-at branch July 2, 2021 08:55
@monperrus monperrus mentioned this pull request Aug 19, 2021
woutersmeenk pushed a commit to woutersmeenk/spoon that referenced this pull request Aug 29, 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.

3 participants