nono, iot is actually good to question things. Having two people review
things is good, and I must say, I think I rushed a bit here and did
misread the code (too much task switching and too little time for Groovy
these days I assume).
So what is myMetaClass for? When I did the review I assumed it is for
the object we "handle". But that is not the case. Instead this is really
the meta class for the handle itself, used by the handle itself.
What does it mean then to have this static? Subclasses will not
influence that and there won't be any per instance meta classes for
this. Now I think it would be no good to have a per instance meta class
on a handle meta class... I mean a meta class of a meta class... well...
In fact I now think we should do a different change. If we decide to
have this static, then I guess the class should actually be final.
And then I would suggest writing a static method to get this meta class,
which does also do the init and everything wanting to work with the
myMetaClass should use this method then. This actually moves the init of
the meta class to an even later point than it is right now, but that is
fine.
As for concurrency... I think if done this way it is no problem.
Multiple threads might create multiple meta classes and they are
different instances and all, but the will do the same, so in the end it
does not matter which of those is used in the end. The meta class itself
is responsible for proper synchronization and initializaton of anything
that is visible across threads, so no external synchronization is required.
what do you guys think?
bye Jochen
Am 06.01.2016 um 00:27 schrieb John Wagenleitner:
My mistake, I didn't know there was a PR associated that Jochen had
already reviewed. Sorry about that.
On Tue, Jan 5, 2016 at 2:45 PM, Pascal Schumacher
<pascalschumac...@gmx.net <mailto:pascalschumac...@gmx.net>> wrote:
Yes, I agree the change most likely reduces performance, but
increases thread-safety (prevent that different Threads may work
with different Metaclasses etc.)
I do not know enough about this area of the code to judge if the
lack of thread-safety is really a concern.
I just merged the pull request because of Jochens +1 vote.
Am 05.01.2016 um 20:31 schrieb John Wagenleitner:
Not sure but wonder if HandleMetaClass#myMetaClass was static to
avoid having to perform a lookup for each HMC instance. Probably
not a issue but thought I'd bring attention to it.
On Tue, Jan 5, 2016 at 10:13 AM, <pascalschumac...@apache.org
<mailto:pascalschumac...@apache.org>> wrote:
Repository: groovy
Updated Branches:
refs/heads/master 586a316da -> c5f17abbe
Fixing squid:S2444 - Lazy initialization of "static" fields
should be "synchronized"
<snip>
http://git-wip-us.apache.org/repos/asf/groovy/blob/c5f17abb/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
----------------------------------------------------------------------
diff --git
a/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
b/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
index f421131..9167f5c 100644
--- a/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
+++ b/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
@@ -24,7 +24,7 @@ import java.lang.reflect.Method;
public class HandleMetaClass extends DelegatingMetaClass {
private Object object;
- private static MetaClass myMetaClass;
+ private MetaClass myMetaClass;
private static final Object NONE = new Object();
public HandleMetaClass(MetaClass mc) {
<snip>
