Skip to content

Conversation

diguage
Copy link
Contributor

@diguage diguage commented Jun 26, 2020

The setting methods of the two fields declare that the default value is false. So, maybe the assignments should be removed.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 26, 2020
@jhoeller
Copy link
Contributor

While we do not usually assign null explicitly, we often do so for the default values of primitive fields, in particular for bean properties where that value is externally visible. Since this is just a matter of style, I'd rather leave it as-is across the codebase.

Thanks for reaching out, in any case :-)

@jhoeller jhoeller closed this Jun 26, 2020
@diguage
Copy link
Contributor Author

diguage commented Jun 26, 2020

While we do not usually assign null explicitly, we often do so for the default values of primitive fields, in particular for bean properties where that value is externally visible. Since this is just a matter of style, I'd rather leave it as-is across the codebase.

Thanks for reaching out, in any case :-)

Why do not assign false to the two fields? It is clearer.

@jhoeller
Copy link
Contributor

Oops I was missing that part of your case here; I thought it was just about explicit false assignments versus relying on the implicit default. Indeed, there is a mismatch with the javadoc: Since the actual default values are true, we need to fix the javadoc to say so.

@jhoeller
Copy link
Contributor

jhoeller commented Jun 26, 2020

To be clear, the default value of the enforce flag is true for an individually configured init/destroy method e.g. on <bean>, while it is being turned to false in case of a wider setting that comes from a <beans> section (which spans several bean definitions, not all of which might actually have an init and destroy method on their bean classes). Same for classpath-scanned components and shared default settings for those. I'll revise the javadoc to explain that.

Well spotted, thanks for pointing this out!

@diguage
Copy link
Contributor Author

diguage commented Jun 26, 2020

To be clear, the default value of the enforce flag is true for an individually configured init/destroy method e.g. on <bean>, while it is being turned to false in case of a wider setting that comes from a <beans> section (which spans several bean definitions, not all of which might actually have an init and destroy method on their bean classes). Same for classpath-scanned components and shared default settings for those. I'll revise the javadoc to explain that.

Well spotted, thanks for pointing this out!

And I review the PR Remove redundant assignment of default values to volatile fields #25261, the result of the benchmark test shows that use the default value is faster.

If necessary, I can change JavaDoc too.

If you must change the code and doc elsewhere, please go ahead.

@jhoeller
Copy link
Contributor

jhoeller commented Jun 26, 2020

That PR was about default values for volatile fields where it indeed makes a performance difference (which I wasn't aware of before). To be the best of my knowledge, there is no noticeable difference for plain local fields; it is just syntactic style here.

The explanation for the adapted enforceInitMethod/enforceDestroyMethod default values is a bit involved, referring to several other mechanisms, so it's probably best for me to handle it directly. I'll also refine that javadoc in our backport branches.

@diguage diguage deleted the remove-assignments branch July 4, 2020 02:26
@bclozel bclozel removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 18, 2022
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