Skip to content

Conversation

@stephengold
Copy link
Member

No description provided.

@Ali-RS
Copy link
Member

Ali-RS commented Sep 17, 2019

Transferred @pspeed42 comment from the other thread (#1180 (comment))

As for the real changes, isn't it odd to set accessible to true for ALL of the constructors only to then also specifically set it for the retrieved constructor?

Unless I'm missing something:
noArgConstructor.setAccessible(true);

...was enough. And:
AccessibleObject.setAccessible(allConstructors, true);

...is unnecessary.

@stephengold
Copy link
Member Author

stephengold commented Sep 17, 2019

@pspeed42 is correct; that 2nd setAccessible() wasn't necessary. Thank you. I'll remove it.

@pspeed42
Copy link
Contributor

pspeed42 commented Sep 17, 2019

Well, I mean setAccessible() on the constructor you will actually use might be better than calling it on every constructor in the whole class. It also means the find constructor method is doing what it says and not extra stuff.
Edit: Ah "Second setAccessible"... I may have read that wrong.

@stephengold stephengold requested a review from Ali-RS September 17, 2019 18:17
Copy link

@Murph9 Murph9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the new InstantiationException

@pspeed42
Copy link
Contributor

Much better.

@stephengold stephengold merged commit 5eaf653 into jMonkeyEngine:master Sep 18, 2019
@stephengold stephengold added this to the v3.3.0 milestone Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants