You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by oreissig <gi...@git.apache.org> on 2016/03/12 20:00:51 UTC
[GitHub] groovy pull request: make private methods static when they are pla...
GitHub user oreissig opened a pull request:
https://github.com/apache/groovy/pull/290
make private methods static when they are plain functions
As discussed in PR #282 I repropose the changes from there in individual pull requests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/oreissig/groovy static-methods
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/290.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #290
----
commit 2bb021398d080d4e5611125a258148987c20050c
Author: oreissig <or...@gmail.com>
Date: 2016-03-04T21:35:49Z
make private methods static when they are plain functions
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request: make private methods static when they are pla...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/290#discussion_r56243773
--- Diff: subprojects/groovy-jsr223/src/main/java/org/codehaus/groovy/jsr223/GroovyScriptEngineImpl.java ---
@@ -427,7 +427,7 @@ private Object callGlobal(String name, Object[] args, ScriptContext ctx) {
}
// generate a unique name for top-level Script classes
- private synchronized String generateScriptName() {
+ private synchronized static String generateScriptName() {
--- End diff --
You're right, caught it as soon as I hit the comment button.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request: make private methods static when they are pla...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/groovy/pull/290
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request: make private methods static when they are pla...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/290#discussion_r56243450
--- Diff: subprojects/groovy-jsr223/src/main/java/org/codehaus/groovy/jsr223/GroovyScriptEngineImpl.java ---
@@ -427,7 +427,7 @@ private Object callGlobal(String name, Object[] args, ScriptContext ctx) {
}
// generate a unique name for top-level Script classes
- private synchronized String generateScriptName() {
+ private synchronized static String generateScriptName() {
--- End diff --
Nevermind, I actually looked at what the method does and the static it increments. This may actually be a fix. :smile: Though doesn't static usually come before synchronized?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request: make private methods static when they are pla...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/290#discussion_r56242514
--- Diff: subprojects/groovy-jsr223/src/main/java/org/codehaus/groovy/jsr223/GroovyScriptEngineImpl.java ---
@@ -427,7 +427,7 @@ private Object callGlobal(String name, Object[] args, ScriptContext ctx) {
}
// generate a unique name for top-level Script classes
- private synchronized String generateScriptName() {
+ private synchronized static String generateScriptName() {
--- End diff --
This changes the method from being accessed by one thread per instance to one thread per class. May not be an issue given what this method does, but in general I would probably not make this kind of bulk change on any `synchronized` method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request: make private methods static when they are pla...
Posted by oreissig <gi...@git.apache.org>.
Github user oreissig commented on a diff in the pull request:
https://github.com/apache/groovy/pull/290#discussion_r56243128
--- Diff: subprojects/groovy-jsr223/src/main/java/org/codehaus/groovy/jsr223/GroovyScriptEngineImpl.java ---
@@ -427,7 +427,7 @@ private Object callGlobal(String name, Object[] args, ScriptContext ctx) {
}
// generate a unique name for top-level Script classes
- private synchronized String generateScriptName() {
+ private synchronized static String generateScriptName() {
--- End diff --
I actually thought about this one, as it sure is a slight semantic change here. The method increments a counter variable, that itself is static. I don't thing having that synchronized on a per-object basis does make sense, or does it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---