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

feature: add CtModifiable#isTransient and related methods #3670

Merged

Conversation

Artamm
Copy link
Contributor

@Artamm Artamm commented Oct 29, 2020

fix #3651
Implemented modifiers from ModifierKind (Transient, Volatile, Synchronized, Native, Strictfp) for CtModifiable class.

@Artamm Artamm changed the title WIP: feature: CtModifiable#isTransient #3651 Review: feature: CtModifiable#isTransient #3651 Oct 30, 2020
@MartinWitt
Copy link
Collaborator

LGTM, nice work!

@Artamm
Copy link
Contributor Author

Artamm commented Oct 30, 2020

LGTM, nice work!

Thank you!

@monperrus
Copy link
Collaborator

Hi @Artamm
Thanks a lot for your contribution.

In Spoon we aim at strongly testing all functionalities.

Could you add test cases to specify those new methods?

Thanks!

@Artamm
Copy link
Contributor Author

Artamm commented Nov 1, 2020

@monperrus , thank you for your response.
I added some test cases in ModifiersTest. Maybe I should add another test case/ edit what I did?

@monperrus
Copy link
Collaborator

Thanks a lot @Artamm , that's good testing. Do you confirm that we test all those new methods with both "true" and "false"?

@Artamm
Copy link
Contributor Author

Artamm commented Nov 4, 2020

@monperrus, I think yes, these tests are enough to cover all cases for new methods.
Well, I had in mind to add fields in ConcreteClass for additional checks too, but this class extends AbstractClass - so it'll just duplicate the code, IMHO.
If it's needed to add something - let me know, I will add.

@monperrus monperrus changed the title Review: feature: CtModifiable#isTransient #3651 feature: add CtModifiable#isTransient and related methods Nov 5, 2020
@monperrus monperrus merged commit dae5bd2 into INRIA:master Nov 5, 2020
@monperrus
Copy link
Collaborator

Big thanks for your contribution @Artamm and thanks a lot @MartinWitt for the shepherding

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.

Feature: CtModifiable#isTransient
3 participants